Re: [PATCH] NFSv4: use unique client identifiers in network namespaces

[Trond Myklebust <trondmy@hammerspace.com>]

On Mon, 2022-02-28 at 09:43 -0500, Benjamin Coddington wrote:
> On 27 Feb 2022, at 18:50, NeilBrown wrote:
> 
> > On Wed, 09 Feb 2022, Benjamin Coddington wrote:
> > > On 8 Feb 2022, at 15:27, Trond Myklebust wrote:
> > > 
> > > > On Tue, 2022-02-08 at 15:03 -0500, Benjamin Coddington wrote:
> > > > > In order to differentiate client state, assign a random uuid
> > > > > to the
> > > > > uniquifing portion of the client identifier when a network 
> > > > > namespace
> > > > > is
> > > > > created.  Containers may still override this value if they
> > > > > wish to
> > > > > maintain
> > > > > stable client identifiers by writing to
> > > > > /sys/fs/nfs/net/client/identifier,
> > > > > either by udev rules or other means.
> > > > > 
> > > > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > > > > ---
> > > > >  fs/nfs/sysfs.c | 14 ++++++++++++++
> > > > >  1 file changed, 14 insertions(+)
> > > > > 
> > > > > diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
> > > > > index 8cb70755e3c9..9b811323fd7f 100644
> > > > > --- a/fs/nfs/sysfs.c
> > > > > +++ b/fs/nfs/sysfs.c
> > > > > @@ -167,6 +167,18 @@ static struct nfs_netns_client
> > > > > *nfs_netns_client_alloc(struct kobject *parent,
> > > > >         return NULL;
> > > > >  }
> > > > >  
> > > > > +static void assign_unique_clientid(struct nfs_netns_client
> > > > > *clp)
> > > > > +{
> > > > > +       unsigned char client_uuid[16];
> > > > > +       char *uuid_str = kmalloc(UUID_STRING_LEN + 1,
> > > > > GFP_KERNEL);
> > > > > +
> > > > > +       if (uuid_str) {
> > > > > +               generate_random_uuid(client_uuid);
> > > > > +               sprintf(uuid_str, "%pU", 
> > > > > client_uuid);
> > > > > +               rcu_assign_pointer(clp->identifier,
> > > > > uuid_str);
> > > > > +       }
> > > > > +}
> > > > > +
> > > > >  void nfs_netns_sysfs_setup(struct nfs_net *netns, struct net
> > > > > *net)
> > > > >  {
> > > > >         struct nfs_netns_client *clp;
> > > > > @@ -174,6 +186,8 @@ void nfs_netns_sysfs_setup(struct nfs_net
> > > > > *netns,
> > > > > struct net *net)
> > > > >         clp = nfs_netns_client_alloc(nfs_client_kobj, net);
> > > > >         if (clp) {
> > > > >                 netns->nfs_client = clp;
> > > > > +               if (net != &init_net)
> > > > > +                       assign_unique_clientid(clp);
> > > > 
> > > > Why shouldn't this go in nfs_netns_client_alloc? At this point 
> > > > you've
> > > > already published the pointer in netns, so you're prone to
> > > > races.
> > > 
> > > No reason it shouldn't, I'll put it there if that's what you
> > > want.
> > > 
> > > I thought that's why it was RCU-ed, and figured we'd just do it 
> > > before
> > > the
> > > uevent.
> > > 
> > > > Also, why the exclusion of init_net?
> > > 
> > > Because we're unique enough if we're not in a network namespace,
> > > and 
> > > any
> > > additional uniqueness we might need (due to NAT, or cloned VMs) 
> > > /should/
> > > be
> > > getting from udev, as you envisioned.  That way, if we are
> > > getting
> > > uniquified, its from a source that's likely
> > > persistent/deterministic.
> > > If we
> > > just generate a random uniquifier, its going to be different next
> > > boot
> > > if
> > > there's no udev rule and userspace helpers.  That's going to make
> > > things
> > > a
> > > lot worse for simple setups.
> > 
> > I liked this patch at first, but I no longer think it is a good
> > idea.
> > 
> > It is quite possible today for containers to provide sufficient
> > uniqueness simply by setting a unique hostname before the first NFS
> > mount.
> > Quite possibly some containers already do this, and are working
> > perfectly.
> > 
> > If we add new randomness, the they will get a different identifier
> > on
> > each boot.  This is bad for exactly the same reason that it is bad
> > for
> > "net == &init_net".
> > 
> > i.e.  I believe this patch could cause a regression for working
> > sites.
> > The regression may not be immediately obvious, but it may still be
> > there.
> > For this reason, I think this patch should be dropped.
> 
> I agree, Trond please drop this patch.
> 
> 
Since it was already pushed to linux-next, it will have to be reverted.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com