Linux NFS development
 help / color / mirror / Atom feed
* [PATCH] NFSv4: use unique client identifiers in network namespaces
@ 2022-02-08 20:03 Benjamin Coddington
  2022-02-08 20:27 ` Trond Myklebust
  0 siblings, 1 reply; 9+ messages in thread
From: Benjamin Coddington @ 2022-02-08 20:03 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs

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);
 		kobject_uevent(&clp->kobject, KOBJ_ADD);
 	}
 }
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] NFSv4: use unique client identifiers in network namespaces
  2022-02-08 20:03 [PATCH] NFSv4: use unique client identifiers in network namespaces Benjamin Coddington
@ 2022-02-08 20:27 ` Trond Myklebust
  2022-02-08 21:37   ` Benjamin Coddington
  0 siblings, 1 reply; 9+ messages in thread
From: Trond Myklebust @ 2022-02-08 20:27 UTC (permalink / raw)
  To: bcodding@redhat.com, anna.schumaker@netapp.com; +Cc: linux-nfs@vger.kernel.org

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.

Also, why the exclusion of init_net?

>                 kobject_uevent(&clp->kobject, KOBJ_ADD);
>         }
>  }

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] NFSv4: use unique client identifiers in network namespaces
  2022-02-08 20:27 ` Trond Myklebust
@ 2022-02-08 21:37   ` Benjamin Coddington
  2022-02-08 21:47     ` Trond Myklebust
  2022-02-27 23:50     ` NeilBrown
  0 siblings, 2 replies; 9+ messages in thread
From: Benjamin Coddington @ 2022-02-08 21:37 UTC (permalink / raw)
  To: Trond Myklebust; +Cc: anna.schumaker, linux-nfs

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.

Ben


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] NFSv4: use unique client identifiers in network namespaces
  2022-02-08 21:37   ` Benjamin Coddington
@ 2022-02-08 21:47     ` Trond Myklebust
  2022-02-09  0:58       ` Benjamin Coddington
  2022-02-27 23:50     ` NeilBrown
  1 sibling, 1 reply; 9+ messages in thread
From: Trond Myklebust @ 2022-02-08 21:47 UTC (permalink / raw)
  To: bcodding@redhat.com; +Cc: linux-nfs@vger.kernel.org, anna.schumaker@netapp.com

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@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.

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@hammerspace.com



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] NFSv4: use unique client identifiers in network namespaces
  2022-02-08 21:47     ` Trond Myklebust
@ 2022-02-09  0:58       ` Benjamin Coddington
  0 siblings, 0 replies; 9+ messages in thread
From: Benjamin Coddington @ 2022-02-09  0:58 UTC (permalink / raw)
  To: Trond Myklebust; +Cc: linux-nfs, anna.schumaker

On 8 Feb 2022, at 16:47, Trond Myklebust wrote:
> On Tue, 2022-02-08 at 16:37 -0500, Benjamin Coddington wrote:
>> On 8 Feb 2022, at 15:27, Trond Myklebust wrote:
>>> 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?

Yes.

Ben


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] NFSv4: use unique client identifiers in network namespaces
  2022-02-08 21:37   ` Benjamin Coddington
  2022-02-08 21:47     ` Trond Myklebust
@ 2022-02-27 23:50     ` NeilBrown
  2022-02-28 14:43       ` Benjamin Coddington
  1 sibling, 1 reply; 9+ messages in thread
From: NeilBrown @ 2022-02-27 23:50 UTC (permalink / raw)
  To: Benjamin Coddington; +Cc: Trond Myklebust, anna.schumaker, linux-nfs

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.

Thanks,
NeilBrown

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] NFSv4: use unique client identifiers in network namespaces
  2022-02-27 23:50     ` NeilBrown
@ 2022-02-28 14:43       ` Benjamin Coddington
  2022-02-28 15:16         ` Trond Myklebust
  0 siblings, 1 reply; 9+ messages in thread
From: Benjamin Coddington @ 2022-02-28 14:43 UTC (permalink / raw)
  To: NeilBrown, Trond Myklebust; +Cc: anna.schumaker, linux-nfs

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.

Ben


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] NFSv4: use unique client identifiers in network namespaces
  2022-02-28 14:43       ` Benjamin Coddington
@ 2022-02-28 15:16         ` Trond Myklebust
  2022-02-28 15:33           ` Benjamin Coddington
  0 siblings, 1 reply; 9+ messages in thread
From: Trond Myklebust @ 2022-02-28 15:16 UTC (permalink / raw)
  To: bcodding@redhat.com, neilb@suse.de
  Cc: linux-nfs@vger.kernel.org, anna.schumaker@netapp.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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] NFSv4: use unique client identifiers in network namespaces
  2022-02-28 15:16         ` Trond Myklebust
@ 2022-02-28 15:33           ` Benjamin Coddington
  0 siblings, 0 replies; 9+ messages in thread
From: Benjamin Coddington @ 2022-02-28 15:33 UTC (permalink / raw)
  To: Trond Myklebust; +Cc: neilb, linux-nfs, anna.schumaker

On 28 Feb 2022, at 10:16, Trond Myklebust wrote:
> On Mon, 2022-02-28 at 09:43 -0500, Benjamin Coddington wrote:
>> On 27 Feb 2022, at 18:50, NeilBrown wrote:
>>> 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.
>

I'm not seeing it on any branch on linux-next.  I presume no one's pulled
your linux-next branch.  Are you not able to drop it from your linux-next
branch?

Ben


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-02-28 15:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-08 20:03 [PATCH] NFSv4: use unique client identifiers in network namespaces Benjamin Coddington
2022-02-08 20:27 ` Trond Myklebust
2022-02-08 21:37   ` Benjamin Coddington
2022-02-08 21:47     ` Trond Myklebust
2022-02-09  0:58       ` Benjamin Coddington
2022-02-27 23:50     ` NeilBrown
2022-02-28 14:43       ` Benjamin Coddington
2022-02-28 15:16         ` Trond Myklebust
2022-02-28 15:33           ` Benjamin Coddington

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox