> On Nov 4, 2020, at 16:01, David Wysochanski <dwysocha@xxxxxxxxxx> wrote:
>
> On Wed, Nov 4, 2020 at 11:28 AM <trondmy@xxxxxxxxx> wrote:
>>
>> From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>>
>> If the server insists on using the readdir verifiers in order to allow
>> cookies to expire, then we should ensure that we cache the verifier
>> with the cookie, so that we can return an error if the application
>> tries to use the expired cookie.
>>
>> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>> ---
>> fs/nfs/dir.c | 35 +++++++++++++++++++++++------------
>> fs/nfs/inode.c | 7 -------
>> include/linux/nfs_fs.h | 8 +++++++-
>> 3 files changed, 30 insertions(+), 20 deletions(-)
>>
>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
>> index 3b44bef3a1b4..454377228167 100644
>> --- a/fs/nfs/dir.c
>> +++ b/fs/nfs/dir.c
>> @@ -155,6 +155,7 @@ struct nfs_readdir_descriptor {
>> loff_t current_index;
>> loff_t prev_index;
>>
>> + __be32 verf[NFS_DIR_VERIFIER_SIZE];
>> unsigned long dir_verifier;
>> unsigned long timestamp;
>> unsigned long gencount;
>> @@ -466,15 +467,15 @@ static int nfs_readdir_search_array(struct nfs_readdir_descriptor *desc)
>>
>> /* Fill a page with xdr information before transferring to the cache page */
>> static int nfs_readdir_xdr_filler(struct nfs_readdir_descriptor *desc,
>> - u64 cookie, struct page **pages,
>> - size_t bufsize)
>> + __be32 *verf, u64 cookie,
>> + struct page **pages, size_t bufsize,
>> + __be32 *verf_res)
>> {
>> struct inode *inode = file_inode(desc->file);
>> - __be32 verf_res[2];
>> struct nfs_readdir_arg arg = {
>> .dentry = file_dentry(desc->file),
>> .cred = desc->file->f_cred,
>> - .verf = NFS_I(inode)->cookieverf,
>> + .verf = verf,
>> .cookie = cookie,
>> .pages = pages,
>> .page_len = bufsize,
>> @@ -503,8 +504,6 @@ static int nfs_readdir_xdr_filler(struct nfs_readdir_descriptor *desc,
>> }
>> desc->timestamp = timestamp;
>> desc->gencount = gencount;
>> - memcpy(NFS_I(inode)->cookieverf, res.verf,
>> - sizeof(NFS_I(inode)->cookieverf));
>> error:
>> return error;
>> }
>> @@ -770,11 +769,13 @@ static struct page **nfs_readdir_alloc_pages(size_t npages)
>> }
>>
>> static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor *desc,
>> - struct page *page, struct inode *inode)
>> + struct page *page, __be32 *verf_arg,
>> + __be32 *verf_res)
>> {
>> struct page **pages;
>> struct nfs_entry *entry;
>> size_t array_size;
>> + struct inode *inode = file_inode(desc->file);
>> size_t dtsize = NFS_SERVER(inode)->dtsize;
>> int status = -ENOMEM;
>>
>> @@ -801,8 +802,9 @@ static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor *desc,
>>
>> do {
>> unsigned int pglen;
>> - status = nfs_readdir_xdr_filler(desc, entry->cookie,
>> - pages, dtsize);
>> + status = nfs_readdir_xdr_filler(desc, verf_arg, entry->cookie,
>> + pages, dtsize,
>> + verf_res);
>> if (status < 0)
>> break;
>>
>> @@ -854,13 +856,15 @@ static int find_and_lock_cache_page(struct nfs_readdir_descriptor *desc)
>> {
>> struct inode *inode = file_inode(desc->file);
>> struct nfs_inode *nfsi = NFS_I(inode);
>> + __be32 verf[NFS_DIR_VERIFIER_SIZE];
>> int res;
>>
>> desc->page = nfs_readdir_page_get_cached(desc);
>> if (!desc->page)
>> return -ENOMEM;
>> if (nfs_readdir_page_needs_filling(desc->page)) {
>> - res = nfs_readdir_xdr_to_array(desc, desc->page, inode);
>> + res = nfs_readdir_xdr_to_array(desc, desc->page,
>> + nfsi->cookieverf, verf);
>> if (res < 0) {
>> nfs_readdir_page_unlock_and_put_cached(desc);
>> if (res == -EBADCOOKIE || res == -ENOTSYNC) {
>> @@ -870,6 +874,7 @@ static int find_and_lock_cache_page(struct nfs_readdir_descriptor *desc)
>> }
>> return res;
>> }
>> + memcpy(nfsi->cookieverf, verf, sizeof(nfsi->cookieverf));
>> }
>> res = nfs_readdir_search_array(desc);
>> if (res == 0) {
>> @@ -902,6 +907,7 @@ static int readdir_search_pagecache(struct nfs_readdir_descriptor *desc)
>> static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
>> {
>> struct file *file = desc->file;
>> + struct nfs_inode *nfsi = NFS_I(file_inode(file));
>> struct nfs_cache_array *array;
>> unsigned int i = 0;
>>
>> @@ -915,6 +921,7 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
>> desc->eof = true;
>> break;
>> }
>> + memcpy(desc->verf, nfsi->cookieverf, sizeof(desc->verf));
>> if (i < (array->size-1))
>> desc->dir_cookie = array->array[i+1].cookie;
>> else
>> @@ -949,8 +956,8 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
>> static int uncached_readdir(struct nfs_readdir_descriptor *desc)
>> {
>> struct page *page = NULL;
>> + __be32 verf[NFS_DIR_VERIFIER_SIZE];
>> int status;
>> - struct inode *inode = file_inode(desc->file);
>>
>> dfprintk(DIRCACHE, "NFS: uncached_readdir() searching for cookie %Lu\n",
>> (unsigned long long)desc->dir_cookie);
>> @@ -967,7 +974,7 @@ static int uncached_readdir(struct nfs_readdir_descriptor *desc)
>> desc->duped = 0;
>>
>> nfs_readdir_page_init_array(page, desc->dir_cookie);
>> - status = nfs_readdir_xdr_to_array(desc, page, inode);
>> + status = nfs_readdir_xdr_to_array(desc, page, desc->verf, verf);
>> if (status < 0)
>> goto out_release;
>>
>> @@ -1023,6 +1030,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
>> desc->dup_cookie = dir_ctx->dup_cookie;
>> desc->duped = dir_ctx->duped;
>> desc->attr_gencount = dir_ctx->attr_gencount;
>> + memcpy(desc->verf, dir_ctx->verf, sizeof(desc->verf));
>> spin_unlock(&file->f_lock);
>>
>> do {
>> @@ -1061,6 +1069,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
>> dir_ctx->dup_cookie = desc->dup_cookie;
>> dir_ctx->duped = desc->duped;
>> dir_ctx->attr_gencount = desc->attr_gencount;
>> + memcpy(dir_ctx->verf, desc->verf, sizeof(dir_ctx->verf));
>> spin_unlock(&file->f_lock);
>>
>> kfree(desc);
>> @@ -1101,6 +1110,8 @@ static loff_t nfs_llseek_dir(struct file *filp, loff_t offset, int whence)
>> dir_ctx->dir_cookie = offset;
>> else
>> dir_ctx->dir_cookie = 0;
>> + if (offset == 0)
>> + memset(dir_ctx->verf, 0, sizeof(dir_ctx->verf));
>> dir_ctx->duped = 0;
>> }
>> spin_unlock(&filp->f_lock);
>
> Thanks for doing these patches!
>
> For some reason this patch does not apply but I get a problem at this hunk.
> Is there a fixup or hunk or two missing from 01/17 ?
> I'm starting at 3cea11cd5e3b (Linux 5.10-rc2).
>
> Problem looks like it's at the spin_unlock - here's what the hunk
> looks like for me:
> fs/nfs/dir.c
> 1092 inode_lock(inode);
> 1093 offset += filp->f_pos;
> 1094 if (offset < 0) {
> 1095 inode_unlock(inode);
> 1096 return -EINVAL;
> 1097 }
> 1098 }
> 1099 if (offset != filp->f_pos) {
> 1100 filp->f_pos = offset;
> 1101 if (nfs_readdir_use_cookie(filp))
> 1102 dir_ctx->dir_cookie = offset;
> 1103 else
> 1104 dir_ctx->dir_cookie = 0;
> 1105 dir_ctx->duped = 0;
> 1106 }
> 1107 inode_unlock(inode);
> 1108 return offset;
> 1109 }
>
Yes, it depends on the patch "[PATCH 1/2] NFS: Remove unnecessary inode locking in nfs_llseek_dir()” that I sent out to the list on Oct 30.
I can include that in future updates of this patchset.
_________________________________
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
