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@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.
I agree, Trond please drop this patch.
Ben
