[[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.
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.
]]
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.
Signed-off-by: NeilBrown <[email protected]>
---
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.
*/
--
2.35.1
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 <[email protected]>
> ---
> 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.
> */
On Thu, 07 Apr 2022, Steve Dickson wrote:
> Hey!
>
> My apologies for taking so long to get to this....
Better late than never !! :-)
>
> 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?
>
https://lore.kernel.org/all/[email protected]/
It hasn't landed, and I didn't expect it to (yet). It is a basis for
discussion and experimentation.
> > 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.
I tend to agree, but it isn't clear how best to flag something in a way
that the flag will actually be seen.
>
> >
> > 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.
ip netns add foo
ip netns exec foo ip link set dev lo up
ip link add veth0 type veth peer name veth1
ip link set veth1 netns foo
ip netns exec foo ifconfig veth1 10.1.1.1/24 up
ifconfig veth0 10.1.1.2/24 up
ip netns exec foo ip route add default via 10.1.1.2
echo 1 > /proc/sys/net/ipv4/ip_forward
iptables -t nat -A POSTROUTING -j SNAT --to-source 10.0.2.15 -s 10.1.1.0/24
## At this point we have a separate network namespace which has network
## connectivity to anything the host can reach.
## The "--to-source" address is the IP address of the host - a qemu
## instance in this case.
# mount in the primary namespace
mount -o vers=4.1 server:/path /mnt
# attempt to mount in the alternate name space
ip netns exec foo mount -o vers=4.1 server:/path /mnt2
Note that "ip netns exec" creates a temporary mount namespace, so when
it exits, /mnt2 will no longer be mounted. If you want to do more than
one thing you might need to
ip netns exec foo /bin/sh my-shell-script
or
ip netns exec foo /bin/bash -i
# in a separate terminal window.
NeilBrown