Re: [PATCH nf-next] netfilter: exthdr: add support for tcp option removal

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

On Mon, Dec 27, 2021 at 03:11:21PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Mon, Dec 20, 2021 at 03:32:47PM +0100, Florian Westphal wrote:
> > > This allows to replace a tcp option with nop padding to selectively disable
> > > a particular tcp option.
> > > 
> > > Optstrip mode is chosen when userspace passes the exthdr expression with
> > > neither a source nor a destination register attribute.
> > > 
> > > This is identical to xtables TCPOPTSTRIP extension.
> > 
> > Is it worth to retain the bitmap approach?
> 
> I don't think so.  For TCPOPTSTRIP it makes sense because
> you can't use multiple targets in one rule.
> 
> I'd rework this to not set BREAK if the option wasn't present
> in the first place, so you could do
> 
> delete tcp option sack-perm delete tcp option timestamp ...
> 
> and so on.
> 
> Let me know if you disagree.

It's OK if you prefer this way. I can see references on the web to
reseting multiple options, not sure if it is actually useful in
practise, in such you can to parse the packet several times.

> I could also rework it so that option comes from sreg instead
> of imm, but i could not find a use-case where having the option number
> coming from a map lookup would make sense.
> 
> > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > > ---
> > >  proposed userspace syntax is:
> > > 
> > >  nft add rule f in delete tcp option sack-perm
> > 
> >    nft add rule f in tcp option reset sack-perm,...
> 
> Why 'reset'?  My initial version had 'remove' but 'delete'
> already exists as a token so it was simpler.

'reset' also exists as a token. This is setting to nop, I just though
reset might make more sense, there might be a nice to really remove
TCP options in the future (costful but paranoid scenario, an observer
can spot options that have been nop)