Re: Netfilter: match "tcp option" with the same type present multiple times

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

Hi Fernando,

On Wed, Oct 07, 2026 at 10:01:51AM +0200, Fernando Fernandez Mancera wrote:
> 
> 
> On 10/7/26 1:29 AM, Pablo Neira Ayuso wrote:
> > Hi Matthieu,
> > 
> > On Wed, Sep 30, 2026 at 08:33:36PM +0200, Matthieu Baerts wrote:
> > > Hi Pablo,
> > > 
> > > Thank you for your reply!
> > > 
> > > On 29/09/2026 14:03, Pablo Neira Ayuso wrote:
> > > > On Tue, Sep 29, 2026 at 12:40:47PM +0200, Matthieu Baerts wrote:
> > > > > On 29/09/2026 12:00, Pablo Neira Ayuso wrote:
> > > > > > On Tue, Sep 29, 2026 at 11:21:15AM +0200, Matthieu Baerts wrote:
> > > > > > > Hi Pablo,
> > > > > > > 
> > > > > > > Thank you for your reply!
> > > > > > > 
> > > > > > > On 28/09/2026 23:52, Pablo Neira Ayuso wrote:
> > > > > > > > Hi Mattieu,
> > > > > > > > 
> > > > > > > > On Mon, Sep 28, 2026 at 02:13:16PM +0200, Matthieu Baerts wrote:
> > > > > > > > > Hello Netfilter devs,
> > > > > > > > > 
> > > > > > > > > First, thank you for maintaining Netfilter in the kernel and the
> > > > > > > > > userspace tools.
> > > > > > > > > 
> > > > > > > > > The MPTCP selftests are switching from IPTables to NFTables, and
> > > > > > > > > Clashiko reported that this part of a rule wouldn't match anything:
> > > > > > > > > 
> > > > > > > > >    tcp option mptcp subtype remove-addr drop
> > > > > > > > > 
> > > > > > > > > When an MPTCP REMOVE_ADDR suboption (type 0x30, len >=4, subtype 0x4) is
> > > > > > > > > added to the TCP options, it is added after an MPTCP DSS option (type
> > > > > > > > > 0x30, len >= 8, subtype 0x2). In other words, there will be two MPTCP
> > > > > > > > > (type 30) options in the TCP options. It looks like Netfilter doesn't
> > > > > > > > > handle that, because it stops processing other TCP options when the
> > > > > > > > > expected type is found:
> > > > > > > > > 
> > > > > > > > > net/netfilter/nft_exthdr.c:nft_exthdr_tcp_eval() {
> > > > > > > > >      ...
> > > > > > > > > 	for (i = sizeof(*tcph); i < tcphdr_len - 1; i += optl) {
> > > > > > > > > 		optl = optlen(opt, i);
> > > > > > > > > 
> > > > > > > > > 		if (priv->type != opt[i])
> > > > > > > > > 			continue;
> > > > > > > > > 		...
> > > > > > > > > 		return;  // <== it will look at the first MPTCP option
> > > > > > > > > 	}
> > > > > > > > >      ...
> > > > > > > > > }
> > > > > > > > > 
> > > > > > > > > It looks like it shouldn't stop if the wrong subtype is found, but the
> > > > > > > > > subtype is not compared there if I'm not mistaken. Should there be a fix
> > > > > > > > > to support this case?
> > > > > > > > 
> > > > > > > > Would it work for you if you can specify what TCP option you want to
> > > > > > > > match? ie.
> > > > > > > >      tcp option[2] mptcp subtype remove-addr drop
> > > > > > > >                 ^
> > > > > > > >                 |
> > > > > > > >                 |
> > > > > > > >         allow to specify a match at a given tcp option
> > > > > > > 
> > > > > > > In my specific use-case, yes it would work. Some MPTCP sub-options will
> > > > > > > always be after another one... when the Linux stack is used.
> > > > > > 
> > > > > > This would require a smaller patch.
> > > > > 
> > > > > I understand. But I guess that still means adding a new option.
> > > > > 
> > > > > In my case, it would work, but this might be confusing for people who
> > > > > want to use this filter: they need to know if a suboption can be used
> > > > > with another one. I guess they could have multiple rules to check the
> > > > > presence of a suboption in the first matched type, and in a second one,
> > > > > but that seems confusing. Or maybe not?
> > > > 
> > > > You mean, the might want to validate the entire list of suboptions in
> > > > a particular order, correct? ie. first suboption A then suboption B.
> > > 
> > > Yes, that's one possibility. But initially, I was thinking about having
> > > two rules (or a set?) to match "the MPTCP subtype is either in the first
> > > MPTCP option, or the second one (if any)". (Or nft could always say with
> > > MPTCP by default, check also for a second MPTCP option, if any)
> > > 
> > > > > > > > Or would this be too strict for your use-case and you would prefer
> > > > > > > > that you can match it anywhere?
> > > > > > > 
> > > > > > > I think it would make more sense to match it anywhere. For example, an
> > > > > > > MP_RESET can be used alone, or after an MP_FASTCLOSE. Plus some stacks
> > > > > > > could reorder the options as there is no imposed order.
> > > > > > 
> > > > > > I think this would require a MPTCP subtype parser, ie. make kernel
> > > > > > aware of MPTCP subtypes.
> > > > > 
> > > > > In nft_exthdr_tcp_eval(), why can the comparison not be done there
> > > > > instead of copying the data in a register, and compare later on? If the
> > > > > comparison is done directly for a given type and is different, the code
> > > > > could continue and check for the same type. (Or store in multiple
> > > > > registers.)
> > > > 
> > > > In nftables, the fetch then cmp instructions are splitted, ie. you
> > > > first retrieve then you compare (or make a set lookup with it).
> > > > Builtin comparison should be possible, but it defeats the integration
> > > > with the set infrastructure.
> > > 
> > > OK, I see.
> > > 
> > > If someone gives ...
> > > 
> > >    tcp option mptcp subtype { mp-capable, mp-join, remove-addr } drop
> > > 
> > > ... a bitmap could be used, but then that's specific to MPTCP I suppose.
> > 
> > I think this makes sense from user perspective.
> > 
> > The nft_exthdr expression needs iterate over the whole list of tcp
> > options, then if type is 30, make a set lookup to check if the subtype
> > value is in the set.
> > 
> > Something like:
> > 
> >   - loop
> >   |  r1 <- exthdr     [ if no more tcp options, break loop ]
> >   |  r2 <- lookup(r1) [ if found, break loop ]
> > 
> > but exthdr need to be taugh to resume from the last visited tcp
> > option. A new loop expression would wrap these two exthdr and lookup
> > expression (similar to nft_inner) and store the iteration context (ie.
> > last visited tcp option to continue from there).
> > 
> > exthdr tcp option support needs advertise a new NFT_EXPR_LOOP flag, so
> > it can be used with within this new nft_loop expression.
> > 
> 
> Wouldn't this be too complex? I was thinking in something a bit more generic
> and simpler. E.g a new NFTA_EXTHDR_INNER_TYPE which will work with PRESENT
> flag.

That's fine, this is needed for matching the subtype.

> That way one can match MPTCP options exactly the same way TCP options are
> matched. The implementation would look first for the TCP option 30 and then
> check the subtype, if it matches return 1 otherwise continue.
> 
> This is chained with a CMP_EQ expression.

I think it would be great if we support this too:

    tcp option mptcp subtype { mp-capable, mp-join, remove-addr } drop

and even combine this new feature with maps.

IIUC, TCP option 30 might come several times, one for each subtype,
that is why I suggested the loop thing, because we need to have
control on the iteration over the list of options. But if you design
copes with the requirements that we have discussed or it can be
extended later on, that's fine.

> I have a working patch, let me polish the code and will send it as RFC today
> so we can discuss it. I like this idea because it could be extended to match
> similar situations in IPv6 headers or SCTP.

Yes, this should work with other extensions too.