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;
Message-ID: <852313a3-005e-2771-8be1-888891370533@redhat.com>
Date:   Wed, 6 Apr 2022 11:57:55 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH/RFC] mount.nfs: handle EADDRINUSE from mount(2)
Content-Language: en-US
To:     NeilBrown <neilb@suse.de>,
        Chuck Lever III <chuck.lever@oracle.com>,
        Benjamin Coddington <bcodding@redhat.com>,
        Trond Myklebust <trondmy@hammerspace.com>
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
References: <164816808982.6096.11435363819568479436@noble.neil.brown.name>
From:   Steve Dickson <steved@redhat.com>
In-Reply-To: <164816808982.6096.11435363819568479436@noble.neil.brown.name>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hey!

My apologies for taking so long to get to this....

On 3/24/22 8:28 PM, NeilBrown wrote:
> 
> [[This is the followup to the kernel patch I recently posted.
>    It changes the behaviour of incorrectly configured containers to
>    get unique client identities - so lease stealing doesn't happen
>    so data corruption is avoided - but does not provide stable
>    identities, so reboot recovery is not ideal.
Which patch are you referring to and did it make it in?

>    What is best to do when configuration is wrong?  Provide best service
>    possible despite it not being perfect, or provide no service so the
>    config will not get fixed.  I could be swayed either way.
> ]]
Maybe a little both? :-) Flag the broken config and continue on
if possible... but flagging the broken config is more critical... IMHO.

> 
> When NFS filesystems are mounted in different network namespaces, each
> network namespace must provide a different hostname (via accompanying
> UTS namespace) or different identifier (via sysfs).
> 
> If the kernel finds that the identity that it constructs is already in
> use in a different namespace it will fail the mount with EADDRINUSE.
> 
> This patch catches that error and, if the sysfs identifier is unset,
> writes a random string and retries.  This allows the mount to complete
> safely even when misconfigured.  The random string has 128 bits of
> entropy and so is extremely likely to be globally unique.
> 
> A lock is taken on the identifier file, and it is only updated if no
> identifier is set.  Thus two concurrent mount attempts will not generate
> different identities.  The mount is retried in any case as a race may
> have updated the identifier while waiting for the lock.
> 
> This is not an ideal solution as an unclean restart of the host cannot
> be detected by the server except by a lease timeout.  If the identifier
> is configured correctly and is stable across restarts, the server can
> detect the restart immediately.  Consequently a warning message is
> generated to encourage correct configuration.
Just curious... How did you test this patch? I would like
to build an env to generate this type of error.

steved.

> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>   utils/mount/stropts.c | 54 ++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index dbdd11e76b41..84266830b84a 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -32,6 +32,7 @@
>   
>   #include <sys/socket.h>
>   #include <sys/mount.h>
> +#include <sys/file.h>
>   #include <netinet/in.h>
>   #include <arpa/inet.h>
>   
> @@ -749,6 +750,50 @@ out:
>   	return ret;
>   }
>   
> +#define ENTROPY_BITS 128
> +static void set_random_identifier(void)
> +{
> +	int fd = open("/sys/fs/nfs/net/nfs_client/identifier", O_RDWR);
> +	int rfd = -1;
> +	unsigned char rbuf[ENTROPY_BITS / 8];
> +	char buf[sizeof(rbuf)*2 + 2];
> +	int n, rn;
> +	int cnt = 1000;
> +
> +	if (fd < 0)
> +		goto out;
> +	/* wait at most one second */
> +	while (flock(fd, LOCK_EX | LOCK_NB) != 0) {
> +		cnt -= 20;
> +		if (cnt < 0)
> +			goto out;
> +		usleep(20 * 1000);
> +	}
> +	n = read(fd, buf, sizeof(buf)-1);
> +	if (n <= 0)
> +		goto out;
> +	buf[n] = 0;
> +	if (n != 7 || strcmp(buf, "(null)\n") != 0)
> +		/* already set */
> +		goto out;
> +	rfd = open("/dev/urandom", O_RDONLY);
> +	if (rfd < 0)
> +		goto out;
> +	rn = read(rfd, rbuf, sizeof(rbuf));
> +	if (rn < (int)sizeof(rbuf))
> +		goto out;
> +	for (n = 0; n < rn; n++)
> +		snprintf(&buf[n*2], 3, "%02x", rbuf[n]);
> +	strcpy(&buf[n*2], "\n");
> +	lseek(fd, SEEK_SET, 0);
> +	write(fd, buf, strlen(buf));
> +out:
> +	if (rfd >= 0)
> +		close(rfd);
> +	if (fd >= 0)
> +		close(fd);
> +}
> +
>   static int nfs_do_mount_v4(struct nfsmount_info *mi,
>   		struct sockaddr *sap, socklen_t salen)
>   {
> @@ -844,7 +889,14 @@ static int nfs_do_mount_v4(struct nfsmount_info *mi,
>   			progname, extra_opts);
>   
>   	result = nfs_sys_mount(mi, options);
> -
> +	if (!result && errno == EADDRINUSE) {
> +		/* client id is not unique, try to create unique id
> +		 * and try again
> +		 */
> +		set_random_identifier();
> +		xlog_warn("Retry mount with randomized identifier. Please configure a stable identifier.");
> +		result = nfs_sys_mount(mi, options);
> +	}
>   	/*
>   	 * If success, update option string to be recorded in /etc/mtab.
>   	 */