Re: [PATCH v2] netfilter: synproxy: erroneous TCP mss option fixed.

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

On Thu, Jun 27, 2019 at 09:21:09PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Thu, Jun 27, 2019 at 09:00:19PM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > >  		opts.options &= info->options;
> > > > > +		client_mssinfo = opts.mss;
> > > > > +		opts.mss = info->mss;
> > > > 
> > > > No need for this new client_mssinfo variable, right? I mean, you can
> > > > just set:
> > > > 
> > > >         opts.mss = info->mss;
> > > > 
> > > > and use it from synproxy_send_client_synack().
> > > 
> > > I thought that as well but we need both mss values,
> > > the one configured in the target (info->mss) and the
> > > ine received from the peer.
> > > 
> > > The former is what we announce to peer in the syn/ack
> > > (as tcp option), the latter is what we need to encode
> > > in the syncookie (to decode it on cookie ack).
> > 
> > I see, probably place client_mss field into the synproxy_options
> > structure?
> 
> I worked on a fix for this too (Ibrahim was faster), I
> tried to rename opts.mss so we have
> 
> u16 mss_peer;
> u16 mss_configured;
> 
> but I got confused myself as to where which mss is to be used.
> 
> perhaps
> u16 mss_option;
> u16 mss_encode;
> 
> ... would have been better.

I would leave the opts.mss as is by now. Given there will be a
conflict between nf-next and nf, I was trying to minimize the number
of chunks for this fix, but that's just my preference (I'll have to
sort out this it seems).

Then, adding follow up patches to rename fields would be great indeed
as you suggest.