| 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: <CAH2r5msEtpJGTvK2F2TY0jR9hU0e7LYJekiyV1zD7MM+i9Oxgg@mail.gmail.com>
Date: Sun, 4 Feb 2024 15:57:43 -0600
From: Steve French <smfrench@...il.com>
To: meetakshisetiyaoss@...il.com
Cc: pc@...guebit.com, ronniesahlberg@...il.com, sprasad@...rosoft.com,
nspmangalore@...il.com, tom@...pey.com, linux-cifs@...r.kernel.org,
linux-kernel@...r.kernel.org, samba-technical@...ts.samba.org,
bharathsm.hsk@...il.com, msetiya@...rosoft.com
Subject: Re: [PATCH] smb: client: do not defer close open handles to deleted files
This was puzzling because I tried some experiments with deleting files
that were deferred close and noticed that we already making sure that
they weren't deferred close in cifs_unlink by calling
cifs_close_deferred_file_under_dentry(tcon, full_path) even in
Rohith's original code from a few years ago - so I didn't see cases
where we had a file marked for delete that was deferred close. How
did you reproduce that problem?
On Fri, Feb 2, 2024 at 11:39 AM <meetakshisetiyaoss@...il.com> wrote:
>
> From: Meetakshi Setiya <msetiya@...rosoft.com>
>
> When a file/dentry has been deleted before closing all its open
> handles, currently, closing them can add them to the deferred close
> list. This can lead to problems in creating file with the same name
> when the file is re-created before the deferred close completes. This
> issue was seen while reusing a client's already existing lease on a
> file for compound operations and xfstest 591 failed because of the
> deferred close handle that remained valid even after the file was
> deleted and was being reused to create a file with the same name. The
> server in this case returns an error on open with
> STATUS_DELETE_PENDING. Recreating the file would fail till the
> deferred handles are closed (duration specified in closetimeo).
>
> This patch fixes the issue by flagging all open handles for the
> deleted file (file path to be precise) with FILE_DELETED at the end
> of the unlink operation. When doing close in cifs_close for each of
> these handles, check the flag and do not defer close these handles if
> the corresponding filepath has been deleted.
>
> Signed-off-by: Meetakshi Setiya <msetiya@...rosoft.com>
> ---
> fs/smb/client/cifsglob.h | 1 +
> fs/smb/client/cifsproto.h | 4 ++++
> fs/smb/client/file.c | 2 +-
> fs/smb/client/inode.c | 2 ++
> fs/smb/client/misc.c | 22 ++++++++++++++++++++++
> 5 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index decf80131bbe..826da216a9e1 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -1399,6 +1399,7 @@ struct cifs_fid_locks {
> struct list_head locks; /* locks held by fid above */
> };
>
> +#define FILE_DELETED 0x550
> struct cifsFileInfo {
> /* following two lists are protected by tcon->open_file_lock */
> struct list_head tlist; /* pointer to next fid owned by tcon */
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index 770db9026850..67f75497db02 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -292,6 +292,10 @@ extern void cifs_close_all_deferred_files(struct cifs_tcon *cifs_tcon);
>
> extern void cifs_close_deferred_file_under_dentry(struct cifs_tcon *cifs_tcon,
> const char *path);
> +
> +extern void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon
> + *cifs_tcon, const char *path);
> +
> extern struct TCP_Server_Info *
> cifs_get_tcp_session(struct smb3_fs_context *ctx,
> struct TCP_Server_Info *primary_server);
> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> index b75282c204da..4c87ddbe8a5a 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -1085,7 +1085,7 @@ int cifs_close(struct inode *inode, struct file *file)
> if ((cifs_sb->ctx->closetimeo && cinode->oplock == CIFS_CACHE_RHW_FLG)
> && cinode->lease_granted &&
> !test_bit(CIFS_INO_CLOSE_ON_LOCK, &cinode->flags) &&
> - dclose) {
> + dclose && !(cfile->f_flags & FILE_DELETED)) {
> if (test_and_clear_bit(CIFS_INO_MODIFIED_ATTR, &cinode->flags)) {
> inode_set_mtime_to_ts(inode,
> inode_set_ctime_current(inode));
> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> index 24489e1e238a..be5bc01319d8 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -1822,6 +1822,8 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
> cifs_inode = CIFS_I(dir);
> CIFS_I(dir)->time = 0; /* force revalidate of dir as well */
> unlink_out:
> + if (rc == 0)
> + cifs_mark_open_handles_for_deleted_file(tcon, full_path);
> free_dentry_path(page);
> kfree(attrs);
> free_xid(xid);
> diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
> index 0748d7b757b9..ebc867bfc034 100644
> --- a/fs/smb/client/misc.c
> +++ b/fs/smb/client/misc.c
> @@ -853,6 +853,28 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path)
> free_dentry_path(page);
> }
>
> +/*
> + * If a dentry has been deleted, all corresponding open handles should know the
> + * same, so that we do not defer close them.
> + */
> +void
> +cifs_mark_open_handles_for_deleted_file(struct cifs_tcon *tcon, const char
> +*path)
> +{
> + struct cifsFileInfo *cfile;
> + void *page;
> + const char *full_path;
> +
> + page = alloc_dentry_path();
> + spin_lock(&tcon->open_file_lock);
> + list_for_each_entry(cfile, &tcon->openFileList, tlist) {
> + full_path = build_path_from_dentry(cfile->dentry, page);
> + if (strcmp(full_path, path) == 0)
> + cfile->f_flags |= FILE_DELETED;
> + }
> + spin_unlock(&tcon->open_file_lock);
> +}
> +
> /* parses DFS referral V3 structure
> * caller is responsible for freeing target_nodes
> * returns:
> --
> 2.39.2
>
>
--
Thanks,
Steve
Powered by blists - more mailing lists