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.
Thanks,
NeilBrown
