ACK/Cmnt: [SRU][v3][F][J][L][M][PATCH 0/1] net: Avoid address overwrite in kernel_connect

[Thadeu Lima de Souza Cascardo]

On Wed, Sep 13, 2023 at 10:16:29AM +0200, Andrea Righi wrote:
> On Wed, Sep 13, 2023 at 09:59:43AM +0200, Stefan Bader wrote:
> > On 12.09.23 21:49, Khalid Elmously wrote:
> > > BugLink: https://bugs.launchpad.net/bugs/2035163
> > > 
> > > An upstream fix is requested to resolve a use-case where sockaddr gets re-written which causes NFS-breakage.
> > > 
> > > 
> > > Testing:
> > >   - GCP has confirmed that the fix works before proposing it upstream. I have verified basic network sanity with the fix applied. I will also ask them to confirm the fix with the kernel in -proposed.
> > > 
> > > 
> > > Regression potenial:
> > >   - The fix modifies kernel_connect() which can have an effect on all kinds of network connections. The change itself is very minor and only converts a pass-by-reference to a pass-by-value - so the risk is considered minimal.
> > > 
> > > 
> > > 
> > > v2:
> > >   - Added nomination for J/gke as well
> > > 
> > > v3:
> > >   - nominating for all generic kernels
> > > 
> > > 
> > > Jordan Rife (1):
> > >    net: Avoid address overwrite in kernel_connect
> > > 
> > >   net/socket.c | 7 ++++++-
> > >   1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > The READ_ONCE() which was worked around was added in v6.6 when I checked
> > correctly. That was done to avoid problematic compiler optimizations.
> > Mentioning this to set awareness that this might lead to different results
> > than upstream and to watch out for possible socket issues in the cycle this
> > gets applied to.
> > Also, would this not be relevant for the various OEM kernels as well?
> > 
> > Acked-by: Stefan Bader <stefan.bader at canonical.com>
> 
> The READ_ONCE() is provided by:
> 
>  1ded5e5a5931 ("net: annotate data-races around sock->ops")
> 
> Maybe we should apply also this one? Otherwise we have a race with ipv6,
> because IPV6_ADDRFORM seems to change sock->ops without any protection.
> 
> I haven't checked in F/J/L, but in M it's definitely safer to have the
> READ_ONCE() (the whole 1ded5e5a5931 actually).
> 
> -Andrea

There were some similar changes in the past. What I recall is that those were
initially detected by KCSAN, which tries very hard to find such potential data
races. They can happen either due to compiler optimizations or CPU out-of-order
execution. So, it is architecture and compiler version and options dependent.

But, despite this being a worthwhile improvement, I don't think it should block
the merge of this fix. The problems they fix are not related, and one can
happen independent of the other. This fix is not going to make that race more
likely.

The only reason the data race fix as a pre-requisite for the NFS fix is to
avoid one more conflict. But I am assuming that except on Mantic, conflicts are
going to abound anyway.

So, agreed on merging the data race fix on Mantic before applying this one, but
not so much on the stable releases.

Cascardo.