[PATCH cifs-utils] mount.cifs: handle multiple ip addresses per hostname

[PATCH cifs-utils] mount.cifs: handle multiple ip addresses per hostname

[Paulo Alcantara]

Support passing multiple 'ip=' options to specify all ip addresses a
server name was resolved to.

Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
---
 mount.cifs.c   | 44 ++++++++++++++++++++------------------------
 mount.cifs.rst |  8 ++++----
 2 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 7f898bbd215a..988e56b649a2 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -2036,6 +2036,21 @@ restore_privs:
 	return rc;
 }
 
+static void set_ip_params(char *options, size_t options_size, char *addrlist)
+{
+	char *s = addrlist + strlen(addrlist), *q = s;
+	char tmp;
+
+	do {
+		for (; s >= addrlist && *s != ','; s--);
+		tmp = *q;
+		*q = '\0';
+		strlcat(options, *options ? ",ip=" : "ip=", options_size);
+		strlcat(options, s + 1, options_size);
+		*q = tmp;
+	} while (q = s--, s >= addrlist);
+}
+
 int main(int argc, char **argv)
 {
 	int c;
@@ -2043,7 +2058,6 @@ int main(int argc, char **argv)
 	char *mountpoint = NULL;
 	char *options = NULL;
 	char *orig_dev = NULL;
-	char *currentaddress, *nextaddress;
 	char *value = NULL;
 	char *ep = NULL;
 	int rc = 0;
@@ -2201,20 +2215,10 @@ assemble_retry:
 			goto mount_exit;
 	}
 
-	currentaddress = parsed_info->addrlist;
-	nextaddress = strchr(currentaddress, ',');
-	if (nextaddress)
-		*nextaddress++ = '\0';
-
 mount_retry:
 	options[0] = '\0';
-	if (!currentaddress) {
-		fprintf(stderr, "Unable to find suitable address.\n");
-		rc = parsed_info->nofail ? 0 : EX_FAIL;
-		goto mount_exit;
-	}
-	strlcpy(options, "ip=", options_size);
-	strlcat(options, currentaddress, options_size);
+
+	set_ip_params(options, options_size, parsed_info->addrlist);
 
 	strlcat(options, ",unc=\\\\", options_size);
 	strlcat(options, parsed_info->host, options_size);
@@ -2266,17 +2270,9 @@ mount_retry:
 		switch (errno) {
 		case ECONNREFUSED:
 		case EHOSTUNREACH:
-			if (currentaddress) {
-				fprintf(stderr, "mount error(%d): could not connect to %s",
-					errno, currentaddress);
-			}
-			currentaddress = nextaddress;
-			if (currentaddress) {
-				nextaddress = strchr(currentaddress, ',');
-				if (nextaddress)
-					*nextaddress++ = '\0';
-			}
-			goto mount_retry;
+			fprintf(stderr, "mount error(%d): could not connect to: %s", errno, parsed_info->addrlist);
+			rc = parsed_info->nofail ? 0 : EX_FAIL;
+			break;
 		case ENODEV:
 			fprintf(stderr,
 				"mount error: %s filesystem not supported by the system\n", cifs_fstype);
diff --git a/mount.cifs.rst b/mount.cifs.rst
index 9d4446f035b6..89fb5c902f79 100644
--- a/mount.cifs.rst
+++ b/mount.cifs.rst
@@ -166,10 +166,10 @@ dir_mode=arg
   If the server does not support the CIFS Unix extensions this overrides
   the default mode for directories.
 
-ip=arg|addr=arg
-  sets the destination IP address. This option is set automatically if
-  the server name portion of the requested UNC name can be resolved so
-  rarely needs to be specified by the user.
+ip=arg|addr=arg[,ip=arg2|addr=arg2,...]
+  Sets a maximum number of 16 destination IP addresses. This option is
+  set automatically if the server name portion of the requested UNC
+  name can be resolved so rarely needs to be specified by the user.
 
 domain=arg|dom=arg|workgroup=arg
   Sets the domain (workgroup) of the user. If no domains are given,
-- 
2.31.1

Re: [PATCH cifs-utils] mount.cifs: handle multiple ip addresses per hostname

[Aurélien Aptel]

I would put in the commit msg that this requires recent kernel.

We should probably merge kernel code first so we can reference it here
in the commit msg, and say in the man page when did the kernel change.

There can be cases where cifs-utils is more recent than kernel and
mount.cifs will pass all the ip instead of trying them before passing
the good one to the kernel but since it's an old kernel it won't try
them all. We could add an option to enable new behavior or check the
kernel version from within mount.cifs.. thoughts?

Paulo Alcantara <pc@cjr.nz> writes:
>  
> +static void set_ip_params(char *options, size_t options_size, char *addrlist)
> +{
> +	char *s = addrlist + strlen(addrlist), *q = s;
> +	char tmp;
> +
> +	do {
> +		for (; s >= addrlist && *s != ','; s--);
> +		tmp = *q;
> +		*q = '\0';
> +		strlcat(options, *options ? ",ip=" : "ip=", options_size);
> +		strlcat(options, s + 1, options_size);
> +		*q = tmp;
> +	} while (q = s--, s >= addrlist);
> +}

I think you should write this in a clearer way and comment, this is hard
to read. What does addrlist value looks like for example? Why are we
copying things backward?
Why is the last 'q' in the while condition instead of the end of the while block?

I was going to say should we return errors if we truncate the ips, but
none of the mount.cifs.c code checks for truncation so I guess we can
ignore.

> +
>  int main(int argc, char **argv)
>  {
>  	int c;
> @@ -2043,7 +2058,6 @@ int main(int argc, char **argv)
>  	char *mountpoint = NULL;
>  	char *options = NULL;
>  	char *orig_dev = NULL;
> -	char *currentaddress, *nextaddress;
>  	char *value = NULL;
>  	char *ep = NULL;
>  	int rc = 0;
> @@ -2201,20 +2215,10 @@ assemble_retry:
>  			goto mount_exit;
>  	}
>  
> -	currentaddress = parsed_info->addrlist;
> -	nextaddress = strchr(currentaddress, ',');
> -	if (nextaddress)
> -		*nextaddress++ = '\0';
> -
>  mount_retry:
>  	options[0] = '\0';
> -	if (!currentaddress) {
> -		fprintf(stderr, "Unable to find suitable address.\n");
> -		rc = parsed_info->nofail ? 0 : EX_FAIL;
> -		goto mount_exit;
> -	}
> -	strlcpy(options, "ip=", options_size);
> -	strlcat(options, currentaddress, options_size);
> +
> +	set_ip_params(options, options_size, parsed_info->addrlist);
>  
>  	strlcat(options, ",unc=\\\\", options_size);
>  	strlcat(options, parsed_info->host, options_size);
> @@ -2266,17 +2270,9 @@ mount_retry:
>  		switch (errno) {
>  		case ECONNREFUSED:
>  		case EHOSTUNREACH:
> -			if (currentaddress) {
> -				fprintf(stderr, "mount error(%d): could not connect to %s",
> -					errno, currentaddress);
> -			}
> -			currentaddress = nextaddress;
> -			if (currentaddress) {
> -				nextaddress = strchr(currentaddress, ',');
> -				if (nextaddress)
> -					*nextaddress++ = '\0';
> -			}
> -			goto mount_retry;
> +			fprintf(stderr, "mount error(%d): could not connect to: %s", errno, parsed_info->addrlist);
> +			rc = parsed_info->nofail ? 0 : EX_FAIL;
> +			break;
>  		case ENODEV:
>  			fprintf(stderr,
>  				"mount error: %s filesystem not supported by the system\n", cifs_fstype);

Ok, so now we pass all IPs to mount() and we don't retry.
I would add a comment before the break to say that the kernel will try
all the IPs so if we get these errors none of them worked.

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [PATCH cifs-utils] mount.cifs: handle multiple ip addresses per hostname

[Steve French]

On Tue, May 11, 2021 at 12:46 PM Aurélien Aptel <aaptel@suse.com> wrote:
>
>
> I would put in the commit msg that this requires recent kernel.
>
> We should probably merge kernel code first so we can reference it here
> in the commit msg, and say in the man page when did the kernel change.
>
> There can be cases where cifs-utils is more recent than kernel and
> mount.cifs will pass all the ip instead of trying them before passing
> the good one to the kernel but since it's an old kernel it won't try
> them all. We could add an option to enable new behavior or check the
> kernel version from within mount.cifs.. thoughts?

We could at least warn on older cifs.ko (which would be unlikely to
work unless someone backported features but didn't backport the
updated version #) by checking for the version number (2.32? 2.33?)

"cat /proc/fs/cifs/DebugData | grep Version" shows the version number
easily info (there are other ways to retrieve it as well)

-- 
Thanks,

Steve

Re: [PATCH cifs-utils] mount.cifs: handle multiple ip addresses per hostname

[Paulo Alcantara]

Aurélien Aptel <aaptel@suse.com> writes:

> I would put in the commit msg that this requires recent kernel.

Agreed.

> We should probably merge kernel code first so we can reference it here
> in the commit msg, and say in the man page when did the kernel change.

Agreed.

> There can be cases where cifs-utils is more recent than kernel and
> mount.cifs will pass all the ip instead of trying them before passing
> the good one to the kernel but since it's an old kernel it won't try
> them all.

Good point!  Yes, we should handle both cases.

> We could add an option to enable new behavior or check the kernel
> version from within mount.cifs.. thoughts?

I don't like the idea of checking the version because the running kernel
might not have the expected patches.

Perhaps a new option would be better... I'll think more about it.

> Paulo Alcantara <pc@cjr.nz> writes:
>>  
>> +static void set_ip_params(char *options, size_t options_size, char *addrlist)
>> +{
>> +	char *s = addrlist + strlen(addrlist), *q = s;
>> +	char tmp;
>> +
>> +	do {
>> +		for (; s >= addrlist && *s != ','; s--);
>> +		tmp = *q;
>> +		*q = '\0';
>> +		strlcat(options, *options ? ",ip=" : "ip=", options_size);
>> +		strlcat(options, s + 1, options_size);
>> +		*q = tmp;
>> +	} while (q = s--, s >= addrlist);
>> +}
>
> I think you should write this in a clearer way and comment, this is hard
> to read.

That's horrible, indeed.  I'll definitely make it readable in next
version.

> I was going to say should we return errors if we truncate the ips, but
> none of the mount.cifs.c code checks for truncation so I guess we can
> ignore.

IIRC, yes.

Re: [PATCH cifs-utils] mount.cifs: handle multiple ip addresses per hostname

[Pavel Shilovsky]

Hi Paulo,

Are you planning to post another version of this patch? If there is
already one HI which I missed, please let me know.
--
Best regards,
Pavel Shilovsky

вт, 11 мая 2021 г. в 11:20, Paulo Alcantara <pc@cjr.nz>:
>
> Aurélien Aptel <aaptel@suse.com> writes:
>
> > I would put in the commit msg that this requires recent kernel.
>
> Agreed.
>
> > We should probably merge kernel code first so we can reference it here
> > in the commit msg, and say in the man page when did the kernel change.
>
> Agreed.
>
> > There can be cases where cifs-utils is more recent than kernel and
> > mount.cifs will pass all the ip instead of trying them before passing
> > the good one to the kernel but since it's an old kernel it won't try
> > them all.
>
> Good point!  Yes, we should handle both cases.
>
> > We could add an option to enable new behavior or check the kernel
> > version from within mount.cifs.. thoughts?
>
> I don't like the idea of checking the version because the running kernel
> might not have the expected patches.
>
> Perhaps a new option would be better... I'll think more about it.
>
> > Paulo Alcantara <pc@cjr.nz> writes:
> >>
> >> +static void set_ip_params(char *options, size_t options_size, char *addrlist)
> >> +{
> >> +    char *s = addrlist + strlen(addrlist), *q = s;
> >> +    char tmp;
> >> +
> >> +    do {
> >> +            for (; s >= addrlist && *s != ','; s--);
> >> +            tmp = *q;
> >> +            *q = '\0';
> >> +            strlcat(options, *options ? ",ip=" : "ip=", options_size);
> >> +            strlcat(options, s + 1, options_size);
> >> +            *q = tmp;
> >> +    } while (q = s--, s >= addrlist);
> >> +}
> >
> > I think you should write this in a clearer way and comment, this is hard
> > to read.
>
> That's horrible, indeed.  I'll definitely make it readable in next
> version.
>
> > I was going to say should we return errors if we truncate the ips, but
> > none of the mount.cifs.c code checks for truncation so I guess we can
> > ignore.
>
> IIRC, yes.

Re: [PATCH cifs-utils] mount.cifs: handle multiple ip addresses per hostname

[Paulo Alcantara]

Hi Pavel,

Pavel Shilovsky <piastryyy@gmail.com> writes:

> Are you planning to post another version of this patch? If there is
> already one HI which I missed, please let me know.

Not for now, thanks.  I would have to repost the multip support in
cifs.ko and rebase it against current for-next.  Then figure out a way
properly handle the different kernel versions and/or compatibility
features in cifs-utils so we can decide whether or not let the kernel
retry all ip addresses at mount time.

I'll let you know.