Re: [PATCH v2 net-next 1/7] socket: Un-export __sock_create().

[David Laight <david.laight.linux@gmail.com>]

On Mon, 26 May 2025 07:29:07 +0200
Christoph Hellwig <hch@lst.de> wrote:

> On Fri, May 23, 2025 at 11:21:07AM -0700, Kuniyuki Iwashima wrote:
> > Since commit eeb1bd5c40ed ("net: Add a struct net parameter to
> > sock_create_kern"), we no longer need to export __sock_create()
> > and can replace all non-core users with sock_create_kern().
> > 
> > Let's convert them and un-export __sock_create().  
> 
> The changes looks good, but the commit log including subject line
> is rather confusing.  What you do is to replace all uses of
> __sock_create with sock_create_kern, which works because
> sock_create_kern just calls __sock_create with the last argument set
> to 1 as those callers do it.  This then allows marking __sock_create
> static because all outside users are gone.
> 
> Please state that, i.e.
> 
> Subect: use sock_create_kern insteadf of opencoding it
> 
> Replace all callers of __sock_create that set the kernel argument to 1
> with sock_create_kern, which is the improve interface for that.
> Mark __sock_create static now that all users outside of socket.c
> are gone.

I'd also like to see an explicit statement on all these patches
about whether the created sockets hold a reference to the namespace.

I know it is documented in the function definitions, but the issue
has always been that the callers get it wrong.

From what I remember, as this point in the patch series sock_create_kern()
doesn't holds a reference, but by the end of the series it does.
That just has to be a recipe for disaster and pretty much requires the
changes all go through the same tree in one merge window.
But the code touches multiple areas and the changes would normally go through
multiple trees.
So it's going to be hard to get all the acks and the patch accepted.
(Unless you persuade Linus to 'just apply the changes'.

I think you need to look at three merge windows.
1) Add new function(s) for creating user/kernel sockets with/without holding
   a namespace reference.
2) Update all the callers to use the new functions.
3) Delete the old functions.

There is no point modifying the callers twice, and the commits need to
explicitly state whether they want the namespace held or not.

	David