From: ronnie sahlberg <ronniesahlberg@gmail.com>
To: Paulo Alcantara <pc@manguebit.com>
Cc: smfrench@gmail.com, linux-cifs@vger.kernel.org
Subject: Re: [PATCH v2 1/2] cifs: improve checking of DFS links over STATUS_OBJECT_NAME_INVALID
Date: Wed, 1 Mar 2023 08:20:51 +1000 [thread overview]
Message-ID: <CAN05THSaVcbsqaR=euCr2Me88HkgMO7fHhX7nc21=cRLKdPwyw@mail.gmail.com> (raw)
In-Reply-To: <20230228220155.23394-1-pc@manguebit.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
On Wed, 1 Mar 2023 at 08:15, Paulo Alcantara <pc@manguebit.com> wrote:
>
> Do not map STATUS_OBJECT_NAME_INVALID to -EREMOTE under non-DFS
> shares, or 'nodfs' mounts or CONFIG_CIFS_DFS_UPCALL=n builds.
> Otherwise, in the slow path, get a referral to figure out whether it
> is an actual DFS link.
>
> This could be simply reproduced under a non-DFS share by running the
> following
>
> $ mount.cifs //srv/share /mnt -o ...
> $ cat /mnt/$(printf '\U110000')
> cat: '/mnt/'$'\364\220\200\200': Object is remote
>
> Fixes: c877ce47e137 ("cifs: reduce roundtrips on create/qinfo requests")
> Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> ---
> v1 -> v2: fixed mem leak on PTR_ERR(ref_path) != -EINVAL
>
> fs/cifs/cifsproto.h | 20 ++++++++++----
> fs/cifs/misc.c | 67 +++++++++++++++++++++++++++++++++++++++++++++
> fs/cifs/smb2inode.c | 21 +++++++-------
> fs/cifs/smb2ops.c | 23 +++++++++-------
> 4 files changed, 106 insertions(+), 25 deletions(-)
>
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index b7a36ebd0f2f..20a2f0f3f682 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -667,11 +667,21 @@ static inline int get_dfs_path(const unsigned int xid, struct cifs_ses *ses,
> int match_target_ip(struct TCP_Server_Info *server,
> const char *share, size_t share_len,
> bool *result);
> -
> -int cifs_dfs_query_info_nonascii_quirk(const unsigned int xid,
> - struct cifs_tcon *tcon,
> - struct cifs_sb_info *cifs_sb,
> - const char *dfs_link_path);
> +int cifs_inval_name_dfs_link_error(const unsigned int xid,
> + struct cifs_tcon *tcon,
> + struct cifs_sb_info *cifs_sb,
> + const char *full_path,
> + bool *islink);
> +#else
> +static inline int cifs_inval_name_dfs_link_error(const unsigned int xid,
> + struct cifs_tcon *tcon,
> + struct cifs_sb_info *cifs_sb,
> + const char *full_path,
> + bool *islink)
> +{
> + *islink = false;
> + return 0;
> +}
> #endif
>
> static inline int cifs_create_options(struct cifs_sb_info *cifs_sb, int options)
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index 2905734eb289..0c6c1fc8dae9 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -21,6 +21,7 @@
> #include "cifsfs.h"
> #ifdef CONFIG_CIFS_DFS_UPCALL
> #include "dns_resolve.h"
> +#include "dfs_cache.h"
> #endif
> #include "fs_context.h"
> #include "cached_dir.h"
> @@ -1198,4 +1199,70 @@ int cifs_update_super_prepath(struct cifs_sb_info *cifs_sb, char *prefix)
> cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_USE_PREFIX_PATH;
> return 0;
> }
> +
> +/*
> + * Handle weird Windows SMB server behaviour. It responds with
> + * STATUS_OBJECT_NAME_INVALID code to SMB2 QUERY_INFO request for
> + * "\<server>\<dfsname>\<linkpath>" DFS reference, where <dfsname> contains
> + * non-ASCII unicode symbols.
> + */
> +int cifs_inval_name_dfs_link_error(const unsigned int xid,
> + struct cifs_tcon *tcon,
> + struct cifs_sb_info *cifs_sb,
> + const char *full_path,
> + bool *islink)
> +{
> + struct cifs_ses *ses = tcon->ses;
> + size_t len;
> + char *path;
> + char *ref_path;
> +
> + *islink = false;
> +
> + /*
> + * Fast path - skip check when @full_path doesn't have a prefix path to
> + * look up or tcon is not DFS.
> + */
> + if (strlen(full_path) < 2 || !cifs_sb ||
> + (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_DFS) ||
> + !is_tcon_dfs(tcon) || !ses->server->origin_fullpath)
> + return 0;
> +
> + /*
> + * Slow path - tcon is DFS and @full_path has prefix path, so attempt
> + * to get a referral to figure out whether it is an DFS link.
> + */
> + len = strnlen(tcon->tree_name, MAX_TREE_SIZE + 1) + strlen(full_path) + 1;
> + path = kmalloc(len, GFP_KERNEL);
> + if (!path)
> + return -ENOMEM;
> +
> + scnprintf(path, len, "%s%s", tcon->tree_name, full_path);
> + ref_path = dfs_cache_canonical_path(path + 1, cifs_sb->local_nls,
> + cifs_remap(cifs_sb));
> + kfree(path);
> +
> + if (IS_ERR(ref_path)) {
> + if (PTR_ERR(ref_path) != -EINVAL)
> + return PTR_ERR(ref_path);
> + } else {
> + struct dfs_info3_param *refs = NULL;
> + int num_refs = 0;
> +
> + /*
> + * XXX: we are not using dfs_cache_find() here because we might
> + * end filling all the DFS cache and thus potentially
> + * removing cached DFS targets that the client would eventually
> + * need during failover.
> + */
> + if (ses->server->ops->get_dfs_refer &&
> + !ses->server->ops->get_dfs_refer(xid, ses, ref_path, &refs,
> + &num_refs, cifs_sb->local_nls,
> + cifs_remap(cifs_sb)))
> + *islink = refs[0].server_type == DFS_TYPE_LINK;
> + free_dfs_info_array(refs, num_refs);
> + kfree(ref_path);
> + }
> + return 0;
> +}
> #endif
> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> index 37b4cd59245d..9b956294e864 100644
> --- a/fs/cifs/smb2inode.c
> +++ b/fs/cifs/smb2inode.c
> @@ -527,12 +527,13 @@ int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_sb_info *cifs_sb, const char *full_path,
> struct cifs_open_info_data *data, bool *adjust_tz, bool *reparse)
> {
> - int rc;
> __u32 create_options = 0;
> struct cifsFileInfo *cfile;
> struct cached_fid *cfid = NULL;
> struct kvec err_iov[3] = {};
> int err_buftype[3] = {};
> + bool islink;
> + int rc, rc2;
>
> *adjust_tz = false;
> *reparse = false;
> @@ -580,15 +581,15 @@ int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> SMB2_OP_QUERY_INFO, cfile, NULL, NULL,
> NULL, NULL);
> goto out;
> - } else if (rc != -EREMOTE && IS_ENABLED(CONFIG_CIFS_DFS_UPCALL) &&
> - hdr->Status == STATUS_OBJECT_NAME_INVALID) {
> - /*
> - * Handle weird Windows SMB server behaviour. It responds with
> - * STATUS_OBJECT_NAME_INVALID code to SMB2 QUERY_INFO request
> - * for "\<server>\<dfsname>\<linkpath>" DFS reference,
> - * where <dfsname> contains non-ASCII unicode symbols.
> - */
> - rc = -EREMOTE;
> + } else if (rc != -EREMOTE && hdr->Status == STATUS_OBJECT_NAME_INVALID) {
> + rc2 = cifs_inval_name_dfs_link_error(xid, tcon, cifs_sb,
> + full_path, &islink);
> + if (rc2) {
> + rc = rc2;
> + goto out;
> + }
> + if (islink)
> + rc = -EREMOTE;
> }
> if (rc == -EREMOTE && IS_ENABLED(CONFIG_CIFS_DFS_UPCALL) && cifs_sb &&
> (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_DFS))
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index f79b075f2992..6dfb865ee9d7 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -796,7 +796,6 @@ static int
> smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_sb_info *cifs_sb, const char *full_path)
> {
> - int rc;
> __le16 *utf16_path;
> __u8 oplock = SMB2_OPLOCK_LEVEL_NONE;
> int err_buftype = CIFS_NO_BUFFER;
> @@ -804,6 +803,8 @@ smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
> struct kvec err_iov = {};
> struct cifs_fid fid;
> struct cached_fid *cfid;
> + bool islink;
> + int rc, rc2;
>
> rc = open_cached_dir(xid, tcon, full_path, cifs_sb, true, &cfid);
> if (!rc) {
> @@ -833,15 +834,17 @@ smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
>
> if (unlikely(!hdr || err_buftype == CIFS_NO_BUFFER))
> goto out;
> - /*
> - * Handle weird Windows SMB server behaviour. It responds with
> - * STATUS_OBJECT_NAME_INVALID code to SMB2 QUERY_INFO request
> - * for "\<server>\<dfsname>\<linkpath>" DFS reference,
> - * where <dfsname> contains non-ASCII unicode symbols.
> - */
> - if (rc != -EREMOTE && IS_ENABLED(CONFIG_CIFS_DFS_UPCALL) &&
> - hdr->Status == STATUS_OBJECT_NAME_INVALID)
> - rc = -EREMOTE;
> +
> + if (rc != -EREMOTE && hdr->Status == STATUS_OBJECT_NAME_INVALID) {
> + rc2 = cifs_inval_name_dfs_link_error(xid, tcon, cifs_sb,
> + full_path, &islink);
> + if (rc2) {
> + rc = rc2;
> + goto out;
> + }
> + if (islink)
> + rc = -EREMOTE;
> + }
> if (rc == -EREMOTE && IS_ENABLED(CONFIG_CIFS_DFS_UPCALL) && cifs_sb &&
> (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_DFS))
> rc = -EOPNOTSUPP;
> --
> 2.39.2
>
next prev parent reply other threads:[~2023-02-28 22:21 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-27 13:53 [PATCH 1/2] cifs: improve checking of DFS links over STATUS_OBJECT_NAME_INVALID Paulo Alcantara
2023-02-27 13:53 ` [PATCH 2/2] cifs: prevent data race in cifs_reconnect_tcon() Paulo Alcantara
2023-02-27 22:40 ` Steve French
2023-02-28 0:09 ` Paulo Alcantara
2023-02-28 22:01 ` [PATCH v2 1/2] cifs: improve checking of DFS links over STATUS_OBJECT_NAME_INVALID Paulo Alcantara
2023-02-28 22:01 ` [PATCH v2 2/2] cifs: prevent data race in cifs_reconnect_tcon() Paulo Alcantara
2023-02-28 22:16 ` Steve French
2023-02-28 22:20 ` ronnie sahlberg [this message]
2023-02-28 23:35 ` [PATCH v2 1/2] cifs: improve checking of DFS links over STATUS_OBJECT_NAME_INVALID Steve French
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAN05THSaVcbsqaR=euCr2Me88HkgMO7fHhX7nc21=cRLKdPwyw@mail.gmail.com' \
--to=ronniesahlberg@gmail.com \
--cc=linux-cifs@vger.kernel.org \
--cc=pc@manguebit.com \
--cc=smfrench@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).