netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kevin 'ldir' Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: "netfilter-devel@vger.kernel.org" <netfilter-devel@vger.kernel.org>
Subject: Re: [RFC nf-next v2 1/2] netfilter: connmark: introduce savedscp
Date: Tue, 30 Apr 2019 20:40:41 +0000	[thread overview]
Message-ID: <5E006285-FB1F-4948-87BE-BD1B8D0321E2@darbyshire-bryant.me.uk> (raw)
In-Reply-To: <20190430122913.lyz7qjh5eebx7lpk@salvia>

Hi Pablo,

Thanks for review.  Some answers inline

> On 30 Apr 2019, at 13:29, Pablo Neira Ayuso <pablo@netfilter.org> 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


  reply	other threads:[~2019-04-30 20:41 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-24 14:23 [RFC PATCH 0/1] netfilter: xt_connmark: add savedscp-mark action Kevin 'ldir' Darbyshire-Bryant
2019-03-24 14:23 ` [PATCH 1/1] netfilter: connmark: introduce savedscp Kevin 'ldir' Darbyshire-Bryant
2019-04-08 22:39   ` Pablo Neira Ayuso
2019-04-08 23:16     ` Kevin 'ldir' Darbyshire-Bryant
2019-04-09 14:23       ` [RFC nf-next v2 0/2] xt_connmark: add savedscp-mark action Kevin 'ldir' Darbyshire-Bryant
2019-04-09 14:23         ` [RFC nf-next v2 1/2] netfilter: connmark: introduce savedscp Kevin 'ldir' Darbyshire-Bryant
2019-04-30 12:29           ` Pablo Neira Ayuso
2019-04-30 20:40             ` Kevin 'ldir' Darbyshire-Bryant [this message]
2019-04-09 14:23         ` [RFC nf-next 2/2] iptables: connmark - add savedscp option Kevin 'ldir' Darbyshire-Bryant
2019-12-03 16:06 ` [PATCH 0/1] netfilter: connmark: introduce set-dscpmark Kevin Darbyshire-Bryant
2019-12-03 16:06   ` [PATCH 1/1] " Kevin Darbyshire-Bryant
2019-12-09 23:57     ` Kevin 'ldir' Darbyshire-Bryant
2019-12-05  8:56   ` [PATCH 0/1] " Jeremy Sowden
2019-12-05  9:46     ` Kevin 'ldir' Darbyshire-Bryant
2019-12-06  8:54       ` Jeremy Sowden
2019-12-05 10:49     ` Florian Westphal
2019-12-05 22:00       ` Jeremy Sowden
2019-12-09 21:42   ` [RFC PATCH nf-next] netfilter: conntrack: add support for storing DiffServ code-point as CT mark Jeremy Sowden
2019-12-09 21:42     ` [RFC PATCH nftables] Add "ct dscpmark" conntrack statement Jeremy Sowden
2019-12-09 22:47     ` [RFC PATCH nf-next] netfilter: conntrack: add support for storing DiffServ code-point as CT mark Florian Westphal
2019-12-09 23:23       ` Jeremy Sowden
2019-12-10  1:25         ` Florian Westphal
2019-12-10 11:01           ` Jeremy Sowden
2019-12-10 11:32             ` Florian Westphal
2019-12-10 19:52               ` Jeremy Sowden
2019-12-11 13:01   ` [PATCH nf-next v2] netfilter: connmark: introduce set-dscpmark Kevin Darbyshire-Bryant

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5E006285-FB1F-4948-87BE-BD1B8D0321E2@darbyshire-bryant.me.uk \
    --to=ldir@darbyshire-bryant.me.uk \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=pablo@netfilter.org \
    --subject='Re: [RFC nf-next v2 1/2] netfilter: connmark: introduce savedscp' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).