Re: [RFC nf-next v2 1/2] netfilter: connmark: introduce savedscp

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Tue, Apr 09, 2019 at 02:23:46PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote:
> diff --git a/include/uapi/linux/netfilter/xt_connmark.h b/include/uapi/linux/netfilter/xt_connmark.h
> index 1aa5c955ee1e..24272cac2d37 100644
> --- a/include/uapi/linux/netfilter/xt_connmark.h
> +++ b/include/uapi/linux/netfilter/xt_connmark.h
> @@ -16,7 +16,8 @@
>  enum {
>  	XT_CONNMARK_SET = 0,
>  	XT_CONNMARK_SAVE,
> -	XT_CONNMARK_RESTORE
> +	XT_CONNMARK_RESTORE,
> +	XT_CONNMARK_SAVEDSCP

I'd prefer you implement this in nftables, more comments below.

>  };
>  
>  enum {
> diff --git a/net/netfilter/xt_connmark.c b/net/netfilter/xt_connmark.c
> index 29c38aa7f726..6c63cf476342 100644
> --- a/net/netfilter/xt_connmark.c
> +++ b/net/netfilter/xt_connmark.c
> @@ -42,6 +42,7 @@ connmark_tg_shift(struct sk_buff *skb, const struct xt_connmark_tginfo2 *info)
>  	u_int32_t new_targetmark;
>  	struct nf_conn *ct;
>  	u_int32_t newmark;
> +	u8 dscp;
>  
>  	ct = nf_ct_get(skb, &ctinfo);
>  	if (ct == NULL)
> @@ -74,6 +75,34 @@ connmark_tg_shift(struct sk_buff *skb, const struct xt_connmark_tginfo2 *info)
>  			nf_conntrack_event_cache(IPCT_MARK, ct);
>  		}
>  		break;
> +	case XT_CONNMARK_SAVEDSCP:

Could you add a new revision and a new flag field for this? so it just
adds the dscp to the mark as you need.

> +		if (!info->ctmark)
> +			goto out;
> +
> +		if (skb->protocol == htons(ETH_P_IP)) {
> +			if (skb->len < sizeof(struct iphdr))
> +				goto out;
> +
> +			dscp = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
> +
> +		} else if (skb->protocol == htons(ETH_P_IPV6)) {
> +			if (skb->len < sizeof(struct ipv6hdr))
> +				goto out;

This is already guaranteed to have a valid IP header in place, no need
for this check.

> +
> +			dscp = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
> +
> +		} else { /* protocol doesn't have diffserv - get out! */
> +			goto out;
> +		}
> +
> +		newmark = (ct->mark & ~info->ctmark) ^
> +			  (info->ctmask | (dscp << info->shift_bits));
> +
> +		if (ct->mark != newmark) {
> +			ct->mark = newmark;
> +			nf_conntrack_event_cache(IPCT_MARK, ct);
> +		}
> +		break;
>  	case XT_CONNMARK_RESTORE:
>  		new_targetmark = (ct->mark & info->ctmask);
>  		if (info->shift_dir == D_SHIFT_RIGHT)
> @@ -86,6 +115,7 @@ connmark_tg_shift(struct sk_buff *skb, const struct xt_connmark_tginfo2 *info)
>  		skb->mark = newmark;
>  		break;
>  	}
> +out:
>  	return XT_CONTINUE;
>  }
>  
> -- 
> 2.20.1 (Apple Git-117)
>