On Tue, 2022-02-08 at 16:37 -0500, 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.
If the value is being set from userspace, then it is up to userland to
solve any problems. However if we're also setting a value from the
kernel, then we have to make sure that we don't clobber any changes
made from userspace. AFAICS, the best way to do that is to initialise
before we publish.
>
> > 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.
>
So you're following up with changes to nfs-utils to configure udev?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
