| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAH2r5mt1X3wp89FLUN_VEzA1kCQmu8x8bVAyi0cdG7-b2V=Bmw@mail.gmail.com>
Date: Fri, 22 Mar 2024 09:47:24 -0500
From: Steve French <smfrench@...il.com>
To: Ritvik Budhiraja <budhirajaritviksmb@...il.com>
Cc: sfrench@...ba.org, pc@...guebit.com, ronniesahlberg@...il.com,
sprasad@...rosoft.com, tom@...pey.com, linux-cifs@...r.kernel.org,
samba-technical@...ts.samba.org, linux-kernel@...r.kernel.org,
bharathsm.hsk@...il.com
Subject: Re: [PATCH] Retrying on failed server close
Do you know a repro scenario where you can get the server to return
EAGAIN or EBUSY?
SInce close is also issued from other paths than the one you issued
retries from (_cifsFileInfo_put) - are there other cases we should be
retrying? e.g. error paths in do_create and atomic_open, cifs_open,
cifs_close_dir, find_cifs_entry
Also do you know a scenario where we can repro the negative total open
files count?
On Fri, Mar 22, 2024 at 2:33 AM Ritvik Budhiraja
<budhirajaritviksmb@...il.com> wrote:
>
> Attaching the updated patch
>
>
> On Fri, 15 Mar 2024 at 01:12, Ritvik Budhiraja <budhirajaritviksmb@...il.com> wrote:
>>
>> In the current implementation, CIFS close sends a close to the server
>> and does not check for the success of the server close. This patch adds
>> functionality to check for server close return status and retries
>> in case of an EBUSY or EAGAIN error
>>
>> Signed-off-by: Ritvik Budhiraja <rbudhiraja@...rosoft.com>
>> ---
>> fs/smb/client/cifsfs.c | 11 +++++++
>> fs/smb/client/cifsglob.h | 7 +++--
>> fs/smb/client/file.c | 63 ++++++++++++++++++++++++++++++++++++----
>> fs/smb/client/smb1ops.c | 4 +--
>> fs/smb/client/smb2ops.c | 9 +++---
>> 5 files changed, 80 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
>> index fb368b191eef..e4b2ded86fce 100644
>> --- a/fs/smb/client/cifsfs.c
>> +++ b/fs/smb/client/cifsfs.c
>> @@ -160,6 +160,7 @@ struct workqueue_struct *decrypt_wq;
>> struct workqueue_struct *fileinfo_put_wq;
>> struct workqueue_struct *cifsoplockd_wq;
>> struct workqueue_struct *deferredclose_wq;
>> +struct workqueue_struct *serverclose_wq;
>> __u32 cifs_lock_secret;
>>
>> /*
>> @@ -1890,6 +1891,13 @@ init_cifs(void)
>> goto out_destroy_cifsoplockd_wq;
>> }
>>
>> + serverclose_wq = alloc_workqueue("serverclose",
>> + WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
>> + if (!serverclose_wq) {
>> + rc = -ENOMEM;
>> + goto out_destroy_serverclose_wq;
>> + }
>> +
>> rc = cifs_init_inodecache();
>> if (rc)
>> goto out_destroy_deferredclose_wq;
>> @@ -1964,6 +1972,8 @@ init_cifs(void)
>> destroy_workqueue(decrypt_wq);
>> out_destroy_cifsiod_wq:
>> destroy_workqueue(cifsiod_wq);
>> +out_destroy_serverclose_wq:
>> + destroy_workqueue(serverclose_wq);
>> out_clean_proc:
>> cifs_proc_clean();
>> return rc;
>> @@ -1993,6 +2003,7 @@ exit_cifs(void)
>> destroy_workqueue(cifsoplockd_wq);
>> destroy_workqueue(decrypt_wq);
>> destroy_workqueue(fileinfo_put_wq);
>> + destroy_workqueue(serverclose_wq);
>> destroy_workqueue(cifsiod_wq);
>> cifs_proc_clean();
>> }
>> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
>> index 53c75cfb33ab..c99bc3b3ff56 100644
>> --- a/fs/smb/client/cifsglob.h
>> +++ b/fs/smb/client/cifsglob.h
>> @@ -429,10 +429,10 @@ struct smb_version_operations {
>> /* set fid protocol-specific info */
>> void (*set_fid)(struct cifsFileInfo *, struct cifs_fid *, __u32);
>> /* close a file */
>> - void (*close)(const unsigned int, struct cifs_tcon *,
>> + int (*close)(const unsigned int, struct cifs_tcon *,
>> struct cifs_fid *);
>> /* close a file, returning file attributes and timestamps */
>> - void (*close_getattr)(const unsigned int xid, struct cifs_tcon *tcon,
>> + int (*close_getattr)(const unsigned int xid, struct cifs_tcon *tcon,
>> struct cifsFileInfo *pfile_info);
>> /* send a flush request to the server */
>> int (*flush)(const unsigned int, struct cifs_tcon *, struct cifs_fid *);
>> @@ -1420,6 +1420,7 @@ struct cifsFileInfo {
>> bool invalidHandle:1; /* file closed via session abend */
>> bool swapfile:1;
>> bool oplock_break_cancelled:1;
>> + bool offload:1; /* offload final part of _put to a wq */
>> unsigned int oplock_epoch; /* epoch from the lease break */
>> __u32 oplock_level; /* oplock/lease level from the lease break */
>> int count;
>> @@ -1428,6 +1429,7 @@ struct cifsFileInfo {
>> struct cifs_search_info srch_inf;
>> struct work_struct oplock_break; /* work for oplock breaks */
>> struct work_struct put; /* work for the final part of _put */
>> + struct work_struct serverclose; /* work for serverclose */
>> struct delayed_work deferred;
>> bool deferred_close_scheduled; /* Flag to indicate close is scheduled */
>> char *symlink_target;
>> @@ -2085,6 +2087,7 @@ extern struct workqueue_struct *decrypt_wq;
>> extern struct workqueue_struct *fileinfo_put_wq;
>> extern struct workqueue_struct *cifsoplockd_wq;
>> extern struct workqueue_struct *deferredclose_wq;
>> +extern struct workqueue_struct *serverclose_wq;
>> extern __u32 cifs_lock_secret;
>>
>> extern mempool_t *cifs_mid_poolp;
>> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
>> index c3b8e7091a4d..c1379ec27dcd 100644
>> --- a/fs/smb/client/file.c
>> +++ b/fs/smb/client/file.c
>> @@ -445,6 +445,7 @@ cifs_down_write(struct rw_semaphore *sem)
>> }
>>
>> static void cifsFileInfo_put_work(struct work_struct *work);
>> +void serverclose_work(struct work_struct *work);
>>
>> struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
>> struct tcon_link *tlink, __u32 oplock,
>> @@ -491,6 +492,7 @@ struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
>> cfile->tlink = cifs_get_tlink(tlink);
>> INIT_WORK(&cfile->oplock_break, cifs_oplock_break);
>> INIT_WORK(&cfile->put, cifsFileInfo_put_work);
>> + INIT_WORK(&cfile->serverclose, serverclose_work);
>> INIT_DELAYED_WORK(&cfile->deferred, smb2_deferred_work_close);
>> mutex_init(&cfile->fh_mutex);
>> spin_lock_init(&cfile->file_info_lock);
>> @@ -582,6 +584,40 @@ static void cifsFileInfo_put_work(struct work_struct *work)
>> cifsFileInfo_put_final(cifs_file);
>> }
>>
>> +void serverclose_work(struct work_struct *work)
>> +{
>> + struct cifsFileInfo *cifs_file = container_of(work,
>> + struct cifsFileInfo, serverclose);
>> +
>> + struct cifs_tcon *tcon = tlink_tcon(cifs_file->tlink);
>> +
>> + struct TCP_Server_Info *server = tcon->ses->server;
>> + int rc;
>> + int retries = 0;
>> + int MAX_RETRIES = 4;
>> +
>> + do {
>> + if (server->ops->close_getattr)
>> + rc = server->ops->close_getattr(0, tcon, cifs_file);
>> + else if (server->ops->close)
>> + rc = server->ops->close(0, tcon, &cifs_file->fid);
>> +
>> + if (rc == -EBUSY || rc == -EAGAIN) {
>> + retries++;
>> + msleep(250);
>> + }
>> + } while ((rc == -EBUSY || rc == -EAGAIN) && (retries < MAX_RETRIES)
>> + );
>> +
>> + if (retries == MAX_RETRIES)
>> + printk(KERN_WARNING "[CIFS_CLOSE] Serverclose failed %d times, giving up\n", MAX_RETRIES);
>> +
>> + if (cifs_file->offload)
>> + queue_work(fileinfo_put_wq, &cifs_file->put);
>> + else
>> + cifsFileInfo_put_final(cifs_file);
>> +}
>> +
>> /**
>> * cifsFileInfo_put - release a reference of file priv data
>> *
>> @@ -622,10 +658,13 @@ void _cifsFileInfo_put(struct cifsFileInfo *cifs_file,
>> struct cifs_fid fid = {};
>> struct cifs_pending_open open;
>> bool oplock_break_cancelled;
>> + bool serverclose_offloaded = false;
>>
>> spin_lock(&tcon->open_file_lock);
>> spin_lock(&cifsi->open_file_lock);
>> spin_lock(&cifs_file->file_info_lock);
>> +
>> + cifs_file->offload = offload;
>> if (--cifs_file->count > 0) {
>> spin_unlock(&cifs_file->file_info_lock);
>> spin_unlock(&cifsi->open_file_lock);
>> @@ -667,13 +706,20 @@ void _cifsFileInfo_put(struct cifsFileInfo *cifs_file,
>> if (!tcon->need_reconnect && !cifs_file->invalidHandle) {
>> struct TCP_Server_Info *server = tcon->ses->server;
>> unsigned int xid;
>> + int rc;
>>
>> xid = get_xid();
>> if (server->ops->close_getattr)
>> - server->ops->close_getattr(xid, tcon, cifs_file);
>> + rc = server->ops->close_getattr(xid, tcon, cifs_file);
>> else if (server->ops->close)
>> - server->ops->close(xid, tcon, &cifs_file->fid);
>> + rc = server->ops->close(xid, tcon, &cifs_file->fid);
>> _free_xid(xid);
>> +
>> + if (rc == -EBUSY || rc == -EAGAIN) {
>> + // Server close failed, hence offloading it as an async op
>> + queue_work(serverclose_wq, &cifs_file->serverclose);
>> + serverclose_offloaded = true;
>> + }
>> }
>>
>> if (oplock_break_cancelled)
>> @@ -681,10 +727,15 @@ void _cifsFileInfo_put(struct cifsFileInfo *cifs_file,
>>
>> cifs_del_pending_open(&open);
>>
>> - if (offload)
>> - queue_work(fileinfo_put_wq, &cifs_file->put);
>> - else
>> - cifsFileInfo_put_final(cifs_file);
>> + // if serverclose has been offloaded to wq (on failure), it will
>> + // handle offloading put as well. If serverclose not offloaded,
>> + // we need to handle offloading put here.
>> + if (!serverclose_offloaded) {
>> + if (offload)
>> + queue_work(fileinfo_put_wq, &cifs_file->put);
>> + else
>> + cifsFileInfo_put_final(cifs_file);
>> + }
>> }
>>
>> int cifs_open(struct inode *inode, struct file *file)
>> diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c
>> index a9eaba8083b0..212ec6f66ec6 100644
>> --- a/fs/smb/client/smb1ops.c
>> +++ b/fs/smb/client/smb1ops.c
>> @@ -753,11 +753,11 @@ cifs_set_fid(struct cifsFileInfo *cfile, struct cifs_fid *fid, __u32 oplock)
>> cinode->can_cache_brlcks = CIFS_CACHE_WRITE(cinode);
>> }
>>
>> -static void
>> +static int
>> cifs_close_file(const unsigned int xid, struct cifs_tcon *tcon,
>> struct cifs_fid *fid)
>> {
>> - CIFSSMBClose(xid, tcon, fid->netfid);
>> + return CIFSSMBClose(xid, tcon, fid->netfid);
>> }
>>
>> static int
>> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
>> index 4695433fcf39..1dcd4944958f 100644
>> --- a/fs/smb/client/smb2ops.c
>> +++ b/fs/smb/client/smb2ops.c
>> @@ -1411,14 +1411,14 @@ smb2_set_fid(struct cifsFileInfo *cfile, struct cifs_fid *fid, __u32 oplock)
>> memcpy(cfile->fid.create_guid, fid->create_guid, 16);
>> }
>>
>> -static void
>> +static int
>> smb2_close_file(const unsigned int xid, struct cifs_tcon *tcon,
>> struct cifs_fid *fid)
>> {
>> - SMB2_close(xid, tcon, fid->persistent_fid, fid->volatile_fid);
>> + return SMB2_close(xid, tcon, fid->persistent_fid, fid->volatile_fid);
>> }
>>
>> -static void
>> +static int
>> smb2_close_getattr(const unsigned int xid, struct cifs_tcon *tcon,
>> struct cifsFileInfo *cfile)
>> {
>> @@ -1429,7 +1429,7 @@ smb2_close_getattr(const unsigned int xid, struct cifs_tcon *tcon,
>> rc = __SMB2_close(xid, tcon, cfile->fid.persistent_fid,
>> cfile->fid.volatile_fid, &file_inf);
>> if (rc)
>> - return;
>> + return rc;
>>
>> inode = d_inode(cfile->dentry);
>>
>> @@ -1458,6 +1458,7 @@ smb2_close_getattr(const unsigned int xid, struct cifs_tcon *tcon,
>>
>> /* End of file and Attributes should not have to be updated on close */
>> spin_unlock(&inode->i_lock);
>> + return rc;
>> }
>>
>> static int
>> --
>> 2.34.1
>>
--
Thanks,
Steve
Powered by blists - more mailing lists