| 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: <CAH2r5mtt0aBrWCK3QgkHv7HHv0wpRHeOVM=5aXr=E5MFsmfFfg@mail.gmail.com>
Date: Wed, 6 Mar 2024 01:09:25 -0600
From: Steve French <smfrench@...il.com>
To: meetakshisetiyaoss@...il.com
Cc: sfrench@...ba.org, 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,
Meetakshi Setiya <msetiya@...rosoft.com>
Subject: Re: [PATCH 2/3] smb: client: do not defer close open handles to
deleted files
Merged the three patches into cifs-2.6.git pending testing but I had
to rebase this one. I also had to rebase one of Paulo's patches "smb:
client: move most of reparse point handling code to common file"
If you see anything wrong with the minor rebase let me know.
On Tue, Mar 5, 2024 at 9:44 PM <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) by setting
> status_file_deleted to true in the cifsFileInfo structure. As per
> the information classes specified in MS-FSCC, SMB2 query info
> response from the server has a DeletePending field, set to true
> to indicate that deletion has been requested on that file. If
> this is the case, flag the open handles for this file too.
>
> When doing close in cifs_close for each of these handles, check the
> value of this boolean field 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 | 3 ++-
> fs/smb/client/inode.c | 28 +++++++++++++++++++++++++---
> fs/smb/client/misc.c | 34 ++++++++++++++++++++++++++++++++++
> fs/smb/client/smb2inode.c | 9 ++++++++-
> 6 files changed, 74 insertions(+), 5 deletions(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index 50f7e47c2229..a88c8328b29c 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -1417,6 +1417,7 @@ struct cifsFileInfo {
> bool invalidHandle:1; /* file closed via session abend */
> bool swapfile:1;
> bool oplock_break_cancelled:1;
> + bool status_file_deleted:1; /* file has been deleted */
> unsigned int oplock_epoch; /* epoch from the lease break */
> __u32 oplock_level; /* oplock/lease level from the lease break */
> int count;
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index ef98c840791f..1f46e0db6e92 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -296,6 +296,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 inode *inode,
> + 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..46951f403d31 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -483,6 +483,7 @@ struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
> cfile->uid = current_fsuid();
> cfile->dentry = dget(dentry);
> cfile->f_flags = file->f_flags;
> + cfile->status_file_deleted = false;
> cfile->invalidHandle = false;
> cfile->deferred_close_scheduled = false;
> cfile->tlink = cifs_get_tlink(tlink);
> @@ -1085,7 +1086,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->status_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 3073eac989ea..3242e3b74386 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -893,6 +893,9 @@ cifs_get_file_info(struct file *filp)
> struct cifsFileInfo *cfile = filp->private_data;
> struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> struct TCP_Server_Info *server = tcon->ses->server;
> + struct dentry *dentry = filp->f_path.dentry;
> + void *page = alloc_dentry_path();
> + const unsigned char *path;
>
> if (!server->ops->query_file_info)
> return -ENOSYS;
> @@ -907,7 +910,14 @@ cifs_get_file_info(struct file *filp)
> data.symlink = true;
> data.reparse.tag = IO_REPARSE_TAG_SYMLINK;
> }
> + path = build_path_from_dentry(dentry, page);
> + if (IS_ERR(path)) {
> + free_dentry_path(page);
> + return PTR_ERR(path);
> + }
> cifs_open_info_to_fattr(&fattr, &data, inode->i_sb);
> + if (fattr.cf_flags & CIFS_FATTR_DELETE_PENDING)
> + cifs_mark_open_handles_for_deleted_file(inode, path);
> break;
> case -EREMOTE:
> cifs_create_junction_fattr(&fattr, inode->i_sb);
> @@ -937,6 +947,7 @@ cifs_get_file_info(struct file *filp)
> rc = cifs_fattr_to_inode(inode, &fattr);
> cgfi_exit:
> cifs_free_open_info(&data);
> + free_dentry_path(page);
> free_xid(xid);
> return rc;
> }
> @@ -1075,6 +1086,7 @@ static int reparse_info_to_fattr(struct cifs_open_info_data *data,
> struct kvec rsp_iov, *iov = NULL;
> int rsp_buftype = CIFS_NO_BUFFER;
> u32 tag = data->reparse.tag;
> + struct inode *inode = NULL;
> int rc = 0;
>
> if (!tag && server->ops->query_reparse_point) {
> @@ -1114,8 +1126,12 @@ static int reparse_info_to_fattr(struct cifs_open_info_data *data,
>
> if (tcon->posix_extensions)
> smb311_posix_info_to_fattr(fattr, data, sb);
> - else
> + else {
> cifs_open_info_to_fattr(fattr, data, sb);
> + inode = cifs_iget(sb, fattr);
> + if (inode && fattr->cf_flags & CIFS_FATTR_DELETE_PENDING)
> + cifs_mark_open_handles_for_deleted_file(inode, full_path);
> + }
> out:
> fattr->cf_cifstag = data->reparse.tag;
> free_rsp_buf(rsp_buftype, rsp_iov.iov_base);
> @@ -1170,6 +1186,8 @@ static int cifs_get_fattr(struct cifs_open_info_data *data,
> full_path, fattr);
> } else {
> cifs_open_info_to_fattr(fattr, data, sb);
> + if (fattr->cf_flags & CIFS_FATTR_DELETE_PENDING)
> + cifs_mark_open_handles_for_deleted_file(*inode, full_path);
> }
> break;
> case -EREMOTE:
> @@ -1850,16 +1868,20 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
>
> psx_del_no_retry:
> if (!rc) {
> - if (inode)
> + if (inode) {
> + cifs_mark_open_handles_for_deleted_file(inode, full_path);
> cifs_drop_nlink(inode);
> + }
> } else if (rc == -ENOENT) {
> d_drop(dentry);
> } else if (rc == -EBUSY) {
> if (server->ops->rename_pending_delete) {
> rc = server->ops->rename_pending_delete(full_path,
> dentry, xid);
> - if (rc == 0)
> + if (rc == 0) {
> + cifs_mark_open_handles_for_deleted_file(inode, full_path);
> cifs_drop_nlink(inode);
> + }
> }
> } else if ((rc == -EACCES) && (dosattr == 0) && inode) {
> attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
> diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
> index 0748d7b757b9..9428a0db7718 100644
> --- a/fs/smb/client/misc.c
> +++ b/fs/smb/client/misc.c
> @@ -853,6 +853,40 @@ 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 that
> + * so that we do not defer close them.
> + */
> +void cifs_mark_open_handles_for_deleted_file(struct inode *inode,
> + const char *path)
> +{
> + struct cifsFileInfo *cfile;
> + void *page;
> + const char *full_path;
> + struct cifsInodeInfo *cinode = CIFS_I(inode);
> +
> + page = alloc_dentry_path();
> + spin_lock(&cinode->open_file_lock);
> +
> + /*
> + * note: we need to construct path from dentry and compare only if the
> + * inode has any hardlinks. When number of hardlinks is 1, we can just
> + * mark all open handles since they are going to be from the same file.
> + */
> + if (inode->i_nlink > 1) {
> + list_for_each_entry(cfile, &cinode->openFileList, flist) {
> + full_path = build_path_from_dentry(cfile->dentry, page);
> + if (!IS_ERR(full_path) && strcmp(full_path, path) == 0)
> + cfile->status_file_deleted = true;
> + }
> + } else {
> + list_for_each_entry(cfile, &cinode->openFileList, flist)
> + cfile->status_file_deleted = true;
> + }
> + spin_unlock(&cinode->open_file_lock);
> + free_dentry_path(page);
> +}
> +
> /* parses DFS referral V3 structure
> * caller is responsible for freeing target_nodes
> * returns:
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 69f3442c5b96..429d83d31280 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -561,8 +561,15 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
> case SMB2_OP_DELETE:
> if (rc)
> trace_smb3_delete_err(xid, ses->Suid, tcon->tid, rc);
> - else
> + else {
> + /*
> + * If dentry (hence, inode) is NULL, lease break is going to
> + * take care of degrading leases on handles for deleted files.
> + */
> + if (inode)
> + cifs_mark_open_handles_for_deleted_file(inode, full_path);
> trace_smb3_delete_done(xid, ses->Suid, tcon->tid);
> + }
> break;
> case SMB2_OP_MKDIR:
> if (rc)
> --
> 2.39.2
>
>
--
Thanks,
Steve
Powered by blists - more mailing lists