Received-SPF: softfail (google.com: domain of transitioning linux-nfs-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   "Benjamin Coddington" <bcodding@redhat.com>
To:     "Trond Myklebust" <trondmy@hammerspace.com>
Cc:     anna.schumaker@netapp.com, linux-nfs@vger.kernel.org
Subject: Re: [PATCH] NFSv4: use unique client identifiers in network
 namespaces
Date:   Tue, 08 Feb 2022 16:37:44 -0500
Message-ID: <7CDAEA98-3A8D-459A-80FE-82AA58A4EDA2@redhat.com>
In-Reply-To: <189aba691b341f64f653817c9cdf018ef34ac257.camel@hammerspace.com>
References: <6e05bf321ff50785360e6c339d111db368e7dfda.1644349990.git.bcodding@redhat.com>
 <189aba691b341f64f653817c9cdf018ef34ac257.camel@hammerspace.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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