Re: [PATCH v3 1/1] fs/smb/client/fs_context: Add hostname option for CIFS module to work with domain-based dfs resources with Kerberos authentication

[Paulo Alcantara <pc@manguebit.org>]

Maria Alexeeva <alxvmr@altlinux.org> writes:

> Paths to domain-based dfs resources are defined using the domain name
> of the server in the format:
> \\<DOMAIN.NAME>\<dfsroot>\<path>
>
> The CIFS module, when requesting a TGS, uses the server name
> (<DOMAIN.NAME>) it obtained from the UNC for the initial connection.
> It then composes an SPN that does not match any entities
> in the domain because it is the domain name itself.
>
> To eliminate this behavior, a hostname option is added, which is
> the name of the server to connect to and is used in composing the SPN.
> In the future this option will be used in the cifs-utils development.
>
> Suggested-by: Ivan Volchenko <ivolchenko86@gmail.com>
> Signed-off-by: Maria Alexeeva <alxvmr@altlinux.org>
> ---
>  fs/smb/client/fs_context.c | 35 +++++++++++++++++++++++++++++------
>  fs/smb/client/fs_context.h |  3 +++
>  2 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index d4291d3a9a48..f0d1895fe4bb 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -177,6 +177,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
>  	fsparam_string("password2", Opt_pass2),
>  	fsparam_string("ip", Opt_ip),
>  	fsparam_string("addr", Opt_ip),
> +	fsparam_string("hostname", Opt_hostname),
>  	fsparam_string("domain", Opt_domain),
>  	fsparam_string("dom", Opt_domain),
>  	fsparam_string("srcaddr", Opt_srcaddr),
> @@ -866,16 +867,23 @@ static int smb3_fs_context_validate(struct fs_context *fc)
>  		return -ENOENT;
>  	}
>  
> +	if (ctx->got_opt_hostname) {
> +		kfree(ctx->server_hostname);
> +		ctx->server_hostname = ctx->opt_hostname;
> +		pr_notice("changing server hostname to name provided in hostname= option\n");

This is just too verbose.  Consider using pr_notice_once() or
cifs_dbg(VFS | ONCE, ...).

> +	}
> +
>  	if (!ctx->got_ip) {
>  		int len;
> -		const char *slash;
>  
> -		/* No ip= option specified? Try to get it from UNC */
> -		/* Use the address part of the UNC. */
> -		slash = strchr(&ctx->UNC[2], '\\');
> -		len = slash - &ctx->UNC[2];
> +		/*
> +		 * No ip= option specified? Try to get it from server_hostname
> +		 * Use the address part of the UNC parsed into server_hostname
> +		 * or hostname= option if specified.
> +		 */
> +		len = strlen(ctx->server_hostname);
>  		if (!cifs_convert_address((struct sockaddr *)&ctx->dstaddr,
> -					  &ctx->UNC[2], len)) {
> +					  ctx->server_hostname, len)) {
>  			pr_err("Unable to determine destination address\n");
>  			return -EHOSTUNREACH;
>  		}
> @@ -1591,6 +1599,21 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
>  		}
>  		ctx->got_ip = true;
>  		break;
> +	case Opt_hostname:
> +		if (strnlen(param->string, CIFS_NI_MAXHOST) == CIFS_NI_MAXHOST) {
> +			pr_warn("host name too long\n");
> +			goto cifs_parse_mount_err;
> +		}
> +
> +		kfree(ctx->opt_hostname);
> +		ctx->opt_hostname = kstrdup(param->string, GFP_KERNEL);
> +		if (ctx->opt_hostname == NULL) {
> +			cifs_errorf(fc, "OOM when copying hostname string\n");
> +			goto cifs_parse_mount_err;
> +		}

No need to kstrdup() @param->string.  You can simply replace it with

                ctx->opt_hostname = no_free_ptr(param->string);

> +		cifs_dbg(FYI, "Host name set\n");

When debugging is enabled, there will be messages logged saying that
'hostname=' has been passed, so no need for this debug message.

> +		ctx->got_opt_hostname = true;
> +		break;
>  	case Opt_domain:
>  		if (strnlen(param->string, CIFS_MAX_DOMAINNAME_LEN)
>  				== CIFS_MAX_DOMAINNAME_LEN) {
> diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
> index 7af7cbbe4208..ff1a04661ef5 100644
> --- a/fs/smb/client/fs_context.h
> +++ b/fs/smb/client/fs_context.h
> @@ -184,6 +184,7 @@ enum cifs_param {
>  	Opt_pass,
>  	Opt_pass2,
>  	Opt_ip,
> +	Opt_hostname,
>  	Opt_domain,
>  	Opt_srcaddr,
>  	Opt_iocharset,
> @@ -214,6 +215,7 @@ struct smb3_fs_context {
>  	bool gid_specified;
>  	bool sloppy;
>  	bool got_ip;
> +	bool got_opt_hostname;
>  	bool got_version;
>  	bool got_rsize;
>  	bool got_wsize;
> @@ -226,6 +228,7 @@ struct smb3_fs_context {
>  	char *domainname;
>  	char *source;
>  	char *server_hostname;
> +	char *opt_hostname;
>  	char *UNC;
>  	char *nodename;
>  	char workstation_name[CIFS_MAX_WORKSTATION_LEN];

You're introducing a new field to smb3_fs_context structure but forgot
to update smb3_fs_context_dup() and smb3_cleanup_fs_context_contents().

This will break automounts as well as leak
smb3_fs_context::opt_hostname.