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 <[email protected]>
---
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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> 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
[email protected]
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 <[email protected]>
> >> ---
> >> 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
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 <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
> > > > > ---
> > > > > 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
[email protected]
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