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>]

Иван Волченко <ivolchenko86@gmail.com> writes:

> пт, 27 февр. 2026 г. в 22:46, Paulo Alcantara <pc@manguebit.org>:
>
>> Maria Alexeeva <alxvmr@altlinux.org> writes:
>>
>> > 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, ...).
>>
>>
> I intentionally used pr_notice() here because the message corresponds
> to a mount-time event. Using pr_notice_once() would suppress the log
> after the first mount, which would make subsequent mount operations
> invisible in the logs.
> Since this path is only executed during filesystem mount, it should
> not be a high-frequency path. Can You provide scenarios
> when that verbosity is the problem?

Consider an DFS mount with hundreds of DFS links.  For every DFS link
accessed and automounted, this message would be get logged for every
mount.

Note that the automount will inherit parent mount's fs context, hence it
will have @got_opt_hostname set to true in the new fs context.

>> > +     }
>> > +
>> >       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.
>>
>>
> I use kstrdup similar to other options processing code and to simplify
> further
> processing of the replaced "server_hostname" field (see below).
> I will think about no_free_ptr if it's that important. Is it?

This is an unnecessary memory allocation.  Could you point out the other
places where kstrdup() is being used for @param->string in mainline
kernel?

>> +             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.
>>
>
>
> There is no any leak and there is no need to process "opt_hostname"
> explicitly in smb3_fs_context_dup() and smb3_cleanup_fs_context_contents(),
> because it's just a temporary pointer, that just copied into
> "server_hostname" (if specified, i.e. not null),

What happens if @ctx->opt_hostname was set and an invalid option that
follows it was passed?  IIUC, @ctx->server_hostname will not be set to
@ctx->opt_hostname and smb3_cleanup_fs_context_contents() will be
called, without freeing @ctx->opt_hostname.

Besides, regarding the automount I mentioned above, if
@ctx->got_opt_hostname was set, the new mount will set
@ctx->server_hostname to @ctx->opt_hostname, causing an UAF bug.