Hi Pablo,
Thanks for review. Some answers inline
> On 30 Apr 2019, at 13:29, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
>
> 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.
I will look into this. nftables is new to me.
>
>> };
>>
>> 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.
Not sure I understand this. Do you mean make it part of XT_CONNMARK_SAVE
and have something like ‘info->dscpmask’? If (!info->dscpmask) do dscp
stuff else do the original routine?
>
>> + 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.
Ok, thanks - removing code is easy (and faster) :-)
Once again thanks for your review & time.
>
>> +
>> + 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)
>>
Cheers,
Kevin D-B
gpg: 012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A
