Hi Ritvik,
LGTM
Reviewed-by: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
On Tue, 19 Nov 2024 at 16:26, Ritvik Budhiraja
<budhirajaritviksmb@xxxxxxxxx> wrote:
>
> Attaching the previously sent patch (on which the current is dependent)
> adding +ronniesahlberg@xxxxxxxxx
>
> ------------------------
>
> [PATCH] CIFS: New mount option for cifs.upcall namespace resolution
> From: Ritvik Budhiraja <rbudhiraja@xxxxxxxxxxxxx>
>
> In the current implementation, the SMB filesystem on a mount point can
> trigger upcalls from the kernel to the userspace to enable certain
> functionalities like spnego, dns_resolution, amongst others. These upcalls
> usually either happen in the context of the mount or in the context of an
> application/user. The upcall handler for cifs, cifs.upcall already has
> existing code which switches the namespaces to the caller's namespace
> before handling the upcall. This behaviour is expected for scenarios like
> multiuser mounts, but might not cover all single user scenario with
> services such as Kubernetes, where the mount can happen from different
> locations such as on the host, from an app container, or a driver pod
> which does the mount on behalf of a different pod.
>
> This patch introduces a new mount option called upcall_target, to
> customise the upcall behaviour. upcall_target can take 'mount' and 'app'
> as possible values. This aids use cases like Kubernetes where the mount
> happens on behalf of the application in another container altogether.
> Having this new mount option allows the mount command to specify where the
> upcall should happen: 'mount' for resolving the upcall to the host
> namespace, and 'app' for resolving the upcall to the ns of the calling
> thread. This will enable both the scenarios where the Kerberos credentials
> can be found on the application namespace or the host namespace to which
> just the mount operation is "delegated".
>
> Reviewed-by: Shyam Prasad <shyam.prasad@xxxxxxxxxxxxx>
> Reviewed-by: Bharath S M <bharathsm@xxxxxxxxxxxxx>
> Signed-off-by: Ritvik Budhiraja <rbudhiraja@xxxxxxxxxxxxx>
> ---
> fs/smb/client/cifs_spnego.c | 16 +++++++++++++++
> fs/smb/client/cifsfs.c | 25 ++++++++++++++++++++++++
> fs/smb/client/cifsglob.h | 7 +++++++
> fs/smb/client/connect.c | 20 +++++++++++++++++++
> fs/smb/client/fs_context.c | 39 +++++++++++++++++++++++++++++++++++++
> fs/smb/client/fs_context.h | 10 ++++++++++
> 6 files changed, 117 insertions(+)
>
> diff --git a/fs/smb/client/cifs_spnego.c b/fs/smb/client/cifs_spnego.c
> index af7849e59..28f568b5f 100644
> --- a/fs/smb/client/cifs_spnego.c
> +++ b/fs/smb/client/cifs_spnego.c
> @@ -82,6 +82,9 @@ struct key_type cifs_spnego_key_type = {
> /* strlen of ";pid=0x" */
> #define PID_KEY_LEN 7
>
> +/* strlen of ";upcall_target=" */
> +#define UPCALL_TARGET_KEY_LEN 15
> +
> /* get a key struct with a SPNEGO security blob, suitable for session setup */
> struct key *
> cifs_get_spnego_key(struct cifs_ses *sesInfo,
> @@ -108,6 +111,11 @@ cifs_get_spnego_key(struct cifs_ses *sesInfo,
> if (sesInfo->user_name)
> desc_len += USER_KEY_LEN + strlen(sesInfo->user_name);
>
> + if (sesInfo->upcall_target == UPTARGET_MOUNT)
> + desc_len += UPCALL_TARGET_KEY_LEN + 5; // strlen("mount")
> + else
> + desc_len += UPCALL_TARGET_KEY_LEN + 3; // strlen("app")
> +
> spnego_key = ERR_PTR(-ENOMEM);
> description = kzalloc(desc_len, GFP_KERNEL);
> if (description == NULL)
> @@ -156,6 +164,14 @@ cifs_get_spnego_key(struct cifs_ses *sesInfo,
> dp = description + strlen(description);
> sprintf(dp, ";pid=0x%x", current->pid);
>
> + if (sesInfo->upcall_target == UPTARGET_MOUNT) {
> + dp = description + strlen(description);
> + sprintf(dp, ";upcall_target=mount");
> + } else {
> + dp = description + strlen(description);
> + sprintf(dp, ";upcall_target=app");
> + }
> +
> cifs_dbg(FYI, "key description = %s\n", description);
> saved_cred = override_creds(spnego_cred);
> spnego_key = request_key(&cifs_spnego_key_type, description, "");
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index 2a2523c93..ac89bd199 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -535,6 +535,30 @@ static int cifs_show_devname(struct seq_file *m, struct dentry *root)
> return 0;
> }
>
> +static void
> +cifs_show_upcall_target(struct seq_file *s, struct cifs_sb_info *cifs_sb)
> +{
> + if (cifs_sb->ctx->upcall_target == UPTARGET_UNSPECIFIED) {
> + seq_puts(s, ",upcall_target=app");
> + return;
> + }
> +
> + seq_puts(s, ",upcall_target=");
> +
> + switch (cifs_sb->ctx->upcall_target) {
> + case UPTARGET_APP:
> + seq_puts(s, "app");
> + break;
> + case UPTARGET_MOUNT:
> + seq_puts(s, "mount");
> + break;
> + default:
> + /* shouldn't ever happen */
> + seq_puts(s, "unknown");
> + break;
> + }
> +}
> +
> /*
> * cifs_show_options() is for displaying mount options in /proc/mounts.
> * Not all settable options are displayed but most of the important
> @@ -551,6 +575,7 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
> seq_show_option(s, "vers", tcon->ses->server->vals->version_string);
> cifs_show_security(s, tcon->ses);
> cifs_show_cache_flavor(s, cifs_sb);
> + cifs_show_upcall_target(s, cifs_sb);
>
> if (tcon->no_lease)
> seq_puts(s, ",nolease");
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index 9eae8649f..7878d1197 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -153,6 +153,12 @@ enum securityEnum {
> Kerberos, /* Kerberos via SPNEGO */
> };
>
> +enum upcall_target_enum {
> + UPTARGET_UNSPECIFIED, /* not specified, defaults to app */
> + UPTARGET_MOUNT, /* upcall to the mount namespace */
> + UPTARGET_APP, /* upcall to the application namespace which did the mount */
> +};
> +
> enum cifs_reparse_type {
> CIFS_REPARSE_TYPE_NFS,
> CIFS_REPARSE_TYPE_WSL,
> @@ -1083,6 +1089,7 @@ struct cifs_ses {
> struct session_key auth_key;
> struct ntlmssp_auth *ntlmssp; /* ciphertext, flags, server challenge */
> enum securityEnum sectype; /* what security flavor was specified? */
> + enum upcall_target_enum upcall_target; /* what upcall target was specified? */
> bool sign; /* is signing required? */
> bool domainAuto:1;
> bool expired_pwd; /* track if access denied or expired pwd so can know if need to update */
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 5375b0c1d..57766bca0 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -2352,6 +2352,26 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>
> ses->sectype = ctx->sectype;
> ses->sign = ctx->sign;
> +
> + /*
> + *Explicitly marking upcall_target mount option for easier handling
> + * by cifs_spnego.c and eventually cifs.upcall.c
> + */
> +
> + switch (ctx->upcall_target) {
> + case UPTARGET_UNSPECIFIED: /* default to app */
> + case UPTARGET_APP:
> + ses->upcall_target = UPTARGET_APP;
> + break;
> + case UPTARGET_MOUNT:
> + ses->upcall_target = UPTARGET_MOUNT;
> + break;
> + default:
> + // should never happen
> + ses->upcall_target = UPTARGET_APP;
> + break;
> + }
> +
> ses->local_nls = load_nls(ctx->local_nls->charset);
>
> /* add server as first channel */
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index bc926ab25..2bae49507 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -67,6 +67,12 @@ static const match_table_t cifs_secflavor_tokens = {
> { Opt_sec_err, NULL }
> };
>
> +static const match_table_t cifs_upcall_target = {
> + { Opt_upcall_target_mount, "mount" },
> + { Opt_upcall_target_application, "app" },
> + { Opt_upcall_target_err, NULL }
> +};
> +
> const struct fs_parameter_spec smb3_fs_parameters[] = {
> /* Mount options that take no arguments */
> fsparam_flag_no("user_xattr", Opt_user_xattr),
> @@ -178,6 +184,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
> fsparam_string("sec", Opt_sec),
> fsparam_string("cache", Opt_cache),
> fsparam_string("reparse", Opt_reparse),
> + fsparam_string("upcall_target", Opt_upcalltarget),
>
> /* Arguments that should be ignored */
> fsparam_flag("guest", Opt_ignore),
> @@ -248,6 +255,29 @@ cifs_parse_security_flavors(struct fs_context *fc, char *value, struct smb3_fs_c
> return 0;
> }
>
> +static int
> +cifs_parse_upcall_target(struct fs_context *fc, char *value, struct smb3_fs_context *ctx)
> +{
> + substring_t args[MAX_OPT_ARGS];
> +
> + ctx->upcall_target = UPTARGET_UNSPECIFIED;
> +
> + switch (match_token(value, cifs_upcall_target, args)) {
> + case Opt_upcall_target_mount:
> + ctx->upcall_target = UPTARGET_MOUNT;
> + break;
> + case Opt_upcall_target_application:
> + ctx->upcall_target = UPTARGET_APP;
> + break;
> +
> + default:
> + cifs_errorf(fc, "bad upcall target: %s\n", value);
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> static const match_table_t cifs_cacheflavor_tokens = {
> { Opt_cache_loose, "loose" },
> { Opt_cache_strict, "strict" },
> @@ -1440,6 +1470,10 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
> if (cifs_parse_security_flavors(fc, param->string, ctx) != 0)
> goto cifs_parse_mount_err;
> break;
> + case Opt_upcalltarget:
> + if (cifs_parse_upcall_target(fc, param->string, ctx) != 0)
> + goto cifs_parse_mount_err;
> + break;
> case Opt_cache:
> if (cifs_parse_cache_flavor(fc, param->string, ctx) != 0)
> goto cifs_parse_mount_err;
> @@ -1617,6 +1651,11 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
> }
> /* case Opt_ignore: - is ignored as expected ... */
>
> + if (ctx->multiuser && ctx->upcall_target == UPTARGET_MOUNT) {
> + cifs_errorf(fc, "multiuser mount option not supported with upcalltarget set as 'mount'\n");
> + goto cifs_parse_mount_err;
> + }
> +
> return 0;
>
> cifs_parse_mount_err:
> diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
> index cf577ec0d..e3ceed48c 100644
> --- a/fs/smb/client/fs_context.h
> +++ b/fs/smb/client/fs_context.h
> @@ -61,6 +61,12 @@ enum cifs_sec_param {
> Opt_sec_err
> };
>
> +enum cifs_upcall_target_param {
> + Opt_upcall_target_mount,
> + Opt_upcall_target_application,
> + Opt_upcall_target_err
> +};
> +
> enum cifs_param {
> /* Mount options that take no arguments */
> Opt_user_xattr,
> @@ -114,6 +120,8 @@ enum cifs_param {
> Opt_multichannel,
> Opt_compress,
> Opt_witness,
> + Opt_is_upcall_target_mount,
> + Opt_is_upcall_target_application,
>
> /* Mount options which take numeric value */
> Opt_backupuid,
> @@ -157,6 +165,7 @@ enum cifs_param {
> Opt_sec,
> Opt_cache,
> Opt_reparse,
> + Opt_upcalltarget,
>
> /* Mount options to be ignored */
> Opt_ignore,
> @@ -198,6 +207,7 @@ struct smb3_fs_context {
> umode_t file_mode;
> umode_t dir_mode;
> enum securityEnum sectype; /* sectype requested via mnt opts */
> + enum upcall_target_enum upcall_target; /* where to upcall for mount */
> bool sign; /* was signing requested via mnt opts? */
> bool ignore_signature:1;
> bool retry:1;
> --
> 2.43.0
>
> On Mon, 18 Nov 2024 at 22:08, <budhirajaritviksmb@xxxxxxxxx> wrote:
>>
>> From: Ritvik Budhiraja <rbudhiraja@xxxxxxxxxxxxx>
>>
>> NOTE: This patch is dependent on one of the previously sent patches:
>> [PATCH] CIFS: New mount option for cifs.upcall namespace resolution
>> which introduces a new mount option called upcall_target, to
>> customise the upcall behaviour.
>>
>> Building upon the above patch, the following patch adds functionality
>> to handle upcall_target as a mount option in cifs.upcall. It can have 2 values -
>> mount, app.
>> Having this new mount option allows the mount command to specify where the
>> upcall should happen: 'mount' for resolving the upcall to the host
>> namespace, and 'app' for resolving the upcall to the ns of the calling
>> thread. This will enable both the scenarios where the Kerberos credentials
>> can be found on the application namespace or the host namespace to which
>> just the mount operation is "delegated".
>> This aids use cases like Kubernetes where the mount
>> happens on behalf of the application in another container altogether.
>>
>> Signed-off-by: Ritvik Budhiraja <rbudhiraja@xxxxxxxxxxxxx>
>> ---
>> cifs.upcall.c | 55 +++++++++++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 47 insertions(+), 8 deletions(-)
>>
>> diff --git a/cifs.upcall.c b/cifs.upcall.c
>> index ff6f2bd..a790aca 100644
>> --- a/cifs.upcall.c
>> +++ b/cifs.upcall.c
>> @@ -954,6 +954,13 @@ struct decoded_args {
>> #define MAX_USERNAME_SIZE 256
>> char username[MAX_USERNAME_SIZE + 1];
>>
>> +#define MAX_UPCALL_STRING_LEN 6 /* "mount\0" */
>> + enum upcall_target_enum {
>> + UPTARGET_UNSPECIFIED, /* not specified, defaults to app */
>> + UPTARGET_MOUNT, /* upcall to the mount namespace */
>> + UPTARGET_APP, /* upcall to the application namespace which did the mount */
>> + } upcall_target;
>> +
>> uid_t uid;
>> uid_t creduid;
>> pid_t pid;
>> @@ -970,6 +977,7 @@ struct decoded_args {
>> #define DKD_HAVE_PID 0x20
>> #define DKD_HAVE_CREDUID 0x40
>> #define DKD_HAVE_USERNAME 0x80
>> +#define DKD_HAVE_UPCALL_TARGET 0x100
>> #define DKD_MUSTHAVE_SET (DKD_HAVE_HOSTNAME|DKD_HAVE_VERSION|DKD_HAVE_SEC)
>> int have;
>> };
>> @@ -980,6 +988,7 @@ __decode_key_description(const char *desc, struct decoded_args *arg)
>> size_t len;
>> char *pos;
>> const char *tkn = desc;
>> + arg->upcall_target = UPTARGET_UNSPECIFIED;
>>
>> do {
>> pos = index(tkn, ';');
>> @@ -1078,6 +1087,31 @@ __decode_key_description(const char *desc, struct decoded_args *arg)
>> }
>> arg->have |= DKD_HAVE_VERSION;
>> syslog(LOG_DEBUG, "ver=%d", arg->ver);
>> + } else if (strncmp(tkn, "upcall_target=", 14) == 0) {
>> + if (pos == NULL)
>> + len = strlen(tkn);
>> + else
>> + len = pos - tkn;
>> +
>> + len -= 14;
>> + if (len > MAX_UPCALL_STRING_LEN) {
>> + syslog(LOG_ERR, "upcall_target= value too long for buffer");
>> + return 1;
>> + }
>> + if (strncmp(tkn + 14, "mount", 5) == 0) {
>> + arg->upcall_target = UPTARGET_MOUNT;
>> + syslog(LOG_DEBUG, "upcall_target=mount");
>> + } else if (strncmp(tkn + 14, "app", 3) == 0) {
>> + arg->upcall_target = UPTARGET_APP;
>> + syslog(LOG_DEBUG, "upcall_target=app");
>> + } else {
>> + // Should never happen
>> + syslog(LOG_ERR, "Invalid upcall_target value: %s, defaulting to app",
>> + tkn + 14);
>> + arg->upcall_target = UPTARGET_APP;
>> + syslog(LOG_DEBUG, "upcall_target=app");
>> + }
>> + arg->have |= DKD_HAVE_UPCALL_TARGET;
>> }
>> if (pos == NULL)
>> break;
>> @@ -1441,15 +1475,20 @@ int main(const int argc, char *const argv[])
>> * acceptably in containers, because we'll be looking at the correct
>> * filesystem and have the correct network configuration.
>> */
>> - rc = switch_to_process_ns(arg->pid);
>> - if (rc == -1) {
>> - syslog(LOG_ERR, "unable to switch to process namespace: %s", strerror(errno));
>> - rc = 1;
>> - goto out;
>> + if (arg->upcall_target == UPTARGET_APP || arg->upcall_target == UPTARGET_UNSPECIFIED) {
>> + syslog(LOG_INFO, "upcall_target=app, switching namespaces to application thread");
>> + rc = switch_to_process_ns(arg->pid);
>> + if (rc == -1) {
>> + syslog(LOG_ERR, "unable to switch to process namespace: %s", strerror(errno));
>> + rc = 1;
>> + goto out;
>> + }
>> + if (trim_capabilities(env_probe))
>> + goto out;
>> + } else {
>> + syslog(LOG_INFO, "upcall_target=mount, not switching namespaces to application thread");
>> }
>>
>> - if (trim_capabilities(env_probe))
>> - goto out;
>>
>> /*
>> * The kernel doesn't pass down the gid, so we resort here to scraping
>> @@ -1496,7 +1535,7 @@ int main(const int argc, char *const argv[])
>> * look at the environ file.
>> */
>> env_cachename =
>> - get_cachename_from_process_env(env_probe ? arg->pid : 0);
>> + get_cachename_from_process_env((env_probe && (arg->upcall_target == UPTARGET_APP)) ? arg->pid : 0);
>>
>> rc = setuid(uid);
>> if (rc == -1) {
>> --
>> 2.43.0
>>
