| Message ID | 6e05bf321ff50785360e6c339d111db368e7dfda.1644349990.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSv4: use unique client identifiers in network namespaces | expand |
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); > } > }
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
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?
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
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
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
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.
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
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); } }
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(+)