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@xxxxxxxxxx> > > > > > --- > > > > > 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@xxxxxxxxxxxxxxx