From: Steve French <smfrench@gmail.com>
To: Paulo Alcantara <pc@cjr.nz>
Cc: CIFS <linux-cifs@vger.kernel.org>
Subject: Re: [PATCH 5/7] cifs: fix path comparison and hash calc
Date: Sat, 5 Jun 2021 16:04:22 -0500 [thread overview]
Message-ID: <CAH2r5muf4Tthghai7dSsX4trQRDeCBb53t9H7sjuHqAnWHOCtw@mail.gmail.com> (raw)
In-Reply-To: <20210604222533.4760-6-pc@cjr.nz>
applied the other 6 patches to cifs-2.6.git for-next but this one
wouldn't apply to for-next so I left it out temporarily.
On Fri, Jun 4, 2021 at 5:26 PM Paulo Alcantara <pc@cjr.nz> wrote:
>
> Fix cache lookup and hash calculations when handling paths with
> different cases.
>
> Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> ---
> fs/cifs/dfs_cache.c | 168 ++++++++++++++++++++++++--------------------
> 1 file changed, 93 insertions(+), 75 deletions(-)
>
> diff --git a/fs/cifs/dfs_cache.c b/fs/cifs/dfs_cache.c
> index c52fec3c4092..f7c4874ddc17 100644
> --- a/fs/cifs/dfs_cache.c
> +++ b/fs/cifs/dfs_cache.c
> @@ -424,12 +424,24 @@ int dfs_cache_init(void)
> return rc;
> }
>
> -static inline unsigned int cache_entry_hash(const void *data, int size)
> +static int cache_entry_hash(const void *data, int size, unsigned int *hash)
> {
> - unsigned int h;
> + int i, clen;
> + const unsigned char *s = data;
> + wchar_t c;
> + unsigned int h = 0;
>
> - h = jhash(data, size, 0);
> - return h & (CACHE_HTABLE_SIZE - 1);
> + for (i = 0; i < size; i += clen) {
> + clen = cache_cp->char2uni(&s[i], size - i, &c);
> + if (unlikely(clen < 0)) {
> + cifs_dbg(VFS, "%s: can't convert char\n", __func__);
> + return clen;
> + }
> + c = cifs_toupper(c);
> + h = jhash(&c, sizeof(c), h);
> + }
> + *hash = h % CACHE_HTABLE_SIZE;
> + return 0;
> }
>
> /* Check whether second path component of @path is SYSVOL or NETLOGON */
> @@ -522,9 +534,7 @@ static int copy_ref_data(const struct dfs_info3_param *refs, int numrefs,
> }
>
> /* Allocate a new cache entry */
> -static struct cache_entry *alloc_cache_entry(const char *path,
> - const struct dfs_info3_param *refs,
> - int numrefs)
> +static struct cache_entry *alloc_cache_entry(struct dfs_info3_param *refs, int numrefs)
> {
> struct cache_entry *ce;
> int rc;
> @@ -533,11 +543,9 @@ static struct cache_entry *alloc_cache_entry(const char *path,
> if (!ce)
> return ERR_PTR(-ENOMEM);
>
> - ce->path = kstrdup(path, GFP_KERNEL);
> - if (!ce->path) {
> - kmem_cache_free(cache_slab, ce);
> - return ERR_PTR(-ENOMEM);
> - }
> + ce->path = refs[0].path_name;
> + refs[0].path_name = NULL;
> +
> INIT_HLIST_NODE(&ce->hlist);
> INIT_LIST_HEAD(&ce->tlist);
>
> @@ -579,12 +587,18 @@ static void remove_oldest_entry_locked(void)
> }
>
> /* Add a new DFS cache entry */
> -static int add_cache_entry_locked(const char *path, unsigned int hash,
> - struct dfs_info3_param *refs, int numrefs)
> +static int add_cache_entry_locked(struct dfs_info3_param *refs, int numrefs)
> {
> + int rc;
> struct cache_entry *ce;
> + unsigned int hash;
>
> - ce = alloc_cache_entry(path, refs, numrefs);
> + convert_delimiter(refs[0].path_name, '\\');
> + rc = cache_entry_hash(refs[0].path_name, strlen(refs[0].path_name), &hash);
> + if (rc)
> + return rc;
> +
> + ce = alloc_cache_entry(refs, numrefs);
> if (IS_ERR(ce))
> return PTR_ERR(ce);
>
> @@ -604,57 +618,69 @@ static int add_cache_entry_locked(const char *path, unsigned int hash,
> return 0;
> }
>
> -static struct cache_entry *__lookup_cache_entry(const char *path)
> +/* Check if two DFS paths are equal. @s1 and @s2 are expected to be in @cache_cp's charset */
> +static bool dfs_path_equal(const char *s1, int len1, const char *s2, int len2)
> +{
> + int i, l1, l2;
> + wchar_t c1, c2;
> +
> + if (len1 != len2)
> + return false;
> +
> + for (i = 0; i < len1; i += l1) {
> + l1 = cache_cp->char2uni(&s1[i], len1 - i, &c1);
> + l2 = cache_cp->char2uni(&s2[i], len2 - i, &c2);
> + if (unlikely(l1 < 0 && l2 < 0)) {
> + if (s1[i] != s2[i])
> + return false;
> + l1 = 1;
> + continue;
> + }
> + if (l1 != l2)
> + return false;
> + if (cifs_toupper(c1) != cifs_toupper(c2))
> + return false;
> + }
> + return true;
> +}
> +
> +static struct cache_entry *__lookup_cache_entry(const char *path, unsigned int hash, int len)
> {
> struct cache_entry *ce;
> - unsigned int h;
> - bool found = false;
>
> - h = cache_entry_hash(path, strlen(path));
> -
> - hlist_for_each_entry(ce, &cache_htable[h], hlist) {
> - if (!strcasecmp(path, ce->path)) {
> - found = true;
> + hlist_for_each_entry(ce, &cache_htable[hash], hlist) {
> + if (dfs_path_equal(ce->path, strlen(ce->path), path, len)) {
> dump_ce(ce);
> - break;
> + return ce;
> }
> }
> -
> - if (!found)
> - ce = ERR_PTR(-ENOENT);
> - return ce;
> + return ERR_PTR(-EEXIST);
> }
>
> /*
> - * Find a DFS cache entry in hash table and optionally check prefix path against
> - * @path.
> - * Use whole path components in the match.
> - * Must be called with htable_rw_lock held.
> + * Find a DFS cache entry in hash table and optionally check prefix path against normalized @path.
> *
> - * Return ERR_PTR(-ENOENT) if the entry is not found.
> + * Use whole path components in the match. Must be called with htable_rw_lock held.
> + *
> + * Return ERR_PTR(-EEXIST) if the entry is not found.
> */
> -static struct cache_entry *lookup_cache_entry(const char *path, unsigned int *hash)
> +static struct cache_entry *lookup_cache_entry(const char *path)
> {
> - struct cache_entry *ce = ERR_PTR(-ENOENT);
> - unsigned int h;
> + struct cache_entry *ce;
> int cnt = 0;
> - char *npath;
> - char *s, *e;
> - char sep;
> + const char *s = path, *e;
> + char sep = *s;
> + unsigned int hash;
> + int rc;
>
> - npath = kstrdup(path, GFP_KERNEL);
> - if (!npath)
> - return ERR_PTR(-ENOMEM);
> -
> - s = npath;
> - sep = *npath;
> while ((s = strchr(s, sep)) && ++cnt < 3)
> s++;
>
> if (cnt < 3) {
> - h = cache_entry_hash(path, strlen(path));
> - ce = __lookup_cache_entry(path);
> - goto out;
> + rc = cache_entry_hash(path, strlen(path), &hash);
> + if (rc)
> + return ERR_PTR(rc);
> + return __lookup_cache_entry(path, hash, strlen(path));
> }
> /*
> * Handle paths that have more than two path components and are a complete prefix of the DFS
> @@ -662,36 +688,29 @@ static struct cache_entry *lookup_cache_entry(const char *path, unsigned int *ha
> *
> * See MS-DFSC 3.2.5.5 "Receiving a Root Referral Request or Link Referral Request".
> */
> - h = cache_entry_hash(npath, strlen(npath));
> - e = npath + strlen(npath) - 1;
> + e = path + strlen(path) - 1;
> while (e > s) {
> - char tmp;
> + int len;
>
> /* skip separators */
> while (e > s && *e == sep)
> e--;
> if (e == s)
> - goto out;
> -
> - tmp = *(e+1);
> - *(e+1) = 0;
> -
> - ce = __lookup_cache_entry(npath);
> - if (!IS_ERR(ce)) {
> - h = cache_entry_hash(npath, strlen(npath));
> break;
> - }
>
> - *(e+1) = tmp;
> + len = e + 1 - path;
> + rc = cache_entry_hash(path, len, &hash);
> + if (rc)
> + return ERR_PTR(rc);
> + ce = __lookup_cache_entry(path, hash, len);
> + if (!IS_ERR(ce))
> + return ce;
> +
> /* backward until separator */
> while (e > s && *e != sep)
> e--;
> }
> -out:
> - if (hash)
> - *hash = h;
> - kfree(npath);
> - return ce;
> + return ERR_PTR(-EEXIST);
> }
>
> /**
> @@ -717,7 +736,7 @@ static int update_cache_entry_locked(const char *path, const struct dfs_info3_pa
> struct cache_entry *ce;
> char *s, *th = NULL;
>
> - ce = lookup_cache_entry(path, NULL);
> + ce = lookup_cache_entry(path);
> if (IS_ERR(ce))
> return PTR_ERR(ce);
>
> @@ -767,7 +786,6 @@ static int get_dfs_referral(const unsigned int xid, struct cifs_ses *ses, const
> static int cache_refresh_path(const unsigned int xid, struct cifs_ses *ses, const char *path)
> {
> int rc;
> - unsigned int hash;
> struct cache_entry *ce;
> struct dfs_info3_param *refs = NULL;
> int numrefs = 0;
> @@ -777,7 +795,7 @@ static int cache_refresh_path(const unsigned int xid, struct cifs_ses *ses, cons
>
> down_write(&htable_rw_lock);
>
> - ce = lookup_cache_entry(path, &hash);
> + ce = lookup_cache_entry(path);
> if (!IS_ERR(ce)) {
> if (!cache_entry_expired(ce)) {
> dump_ce(ce);
> @@ -808,7 +826,7 @@ static int cache_refresh_path(const unsigned int xid, struct cifs_ses *ses, cons
> remove_oldest_entry_locked();
> }
>
> - rc = add_cache_entry_locked(path, hash, refs, numrefs);
> + rc = add_cache_entry_locked(refs, numrefs);
> if (!rc)
> atomic_inc(&cache_count);
>
> @@ -940,7 +958,7 @@ int dfs_cache_find(const unsigned int xid, struct cifs_ses *ses, const struct nl
>
> down_read(&htable_rw_lock);
>
> - ce = lookup_cache_entry(npath, NULL);
> + ce = lookup_cache_entry(npath);
> if (IS_ERR(ce)) {
> up_read(&htable_rw_lock);
> rc = PTR_ERR(ce);
> @@ -987,7 +1005,7 @@ int dfs_cache_noreq_find(const char *path, struct dfs_info3_param *ref,
>
> down_read(&htable_rw_lock);
>
> - ce = lookup_cache_entry(path, NULL);
> + ce = lookup_cache_entry(path);
> if (IS_ERR(ce)) {
> rc = PTR_ERR(ce);
> goto out_unlock;
> @@ -1044,7 +1062,7 @@ int dfs_cache_update_tgthint(const unsigned int xid, struct cifs_ses *ses,
>
> down_write(&htable_rw_lock);
>
> - ce = lookup_cache_entry(npath, NULL);
> + ce = lookup_cache_entry(npath);
> if (IS_ERR(ce)) {
> rc = PTR_ERR(ce);
> goto out_unlock;
> @@ -1098,7 +1116,7 @@ int dfs_cache_noreq_update_tgthint(const char *path, const struct dfs_cache_tgt_
>
> down_write(&htable_rw_lock);
>
> - ce = lookup_cache_entry(path, NULL);
> + ce = lookup_cache_entry(path);
> if (IS_ERR(ce)) {
> rc = PTR_ERR(ce);
> goto out_unlock;
> @@ -1147,7 +1165,7 @@ int dfs_cache_get_tgt_referral(const char *path, const struct dfs_cache_tgt_iter
>
> down_read(&htable_rw_lock);
>
> - ce = lookup_cache_entry(path, NULL);
> + ce = lookup_cache_entry(path);
> if (IS_ERR(ce)) {
> rc = PTR_ERR(ce);
> goto out_unlock;
> --
> 2.31.1
>
--
Thanks,
Steve
next prev parent reply other threads:[~2021-06-05 21:05 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-04 22:25 [PATCH 0/7] dfs fixes Paulo Alcantara
2021-06-04 22:25 ` [PATCH 1/7] cifs: do not send tree disconnect to ipc shares Paulo Alcantara
2021-06-04 22:25 ` [PATCH 2/7] cifs: get rid of @noreq param in __dfs_cache_find() Paulo Alcantara
2021-06-04 22:25 ` [PATCH 3/7] cifs: keep referral server sessions alive Paulo Alcantara
2021-06-04 22:25 ` [PATCH 4/7] cifs: handle different charsets in dfs cache Paulo Alcantara
2021-06-04 22:25 ` [PATCH 5/7] cifs: fix path comparison and hash calc Paulo Alcantara
2021-06-05 21:04 ` Steve French [this message]
2021-06-04 22:25 ` [PATCH 6/7] cifs: set a minimum of 2 minutes for refreshing dfs cache Paulo Alcantara
2021-06-04 22:25 ` [PATCH 7/7] cifs: do not share tcp servers with dfs mounts Paulo Alcantara
2021-06-05 21:05 ` [PATCH 0/7] dfs fixes 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=CAH2r5muf4Tthghai7dSsX4trQRDeCBb53t9H7sjuHqAnWHOCtw@mail.gmail.com \
--to=smfrench@gmail.com \
--cc=linux-cifs@vger.kernel.org \
--cc=pc@cjr.nz \
/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).