Re: [PATCH 2/3] smb: client: do not defer close open handles to deleted files

Paulo Alcantara
2024-03-06T14:32:00Z

meetakshisetiyaoss@gmail.com writes:

> 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;

This is unnecessary as @cfile is kzalloc()'d.

> 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;

You're leaking @page if above condition is true.

> @@ -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);

Now you're leaking @data and @fid if above condition is true.

> + }
> 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)

You shouldn't ignore if cifs_iget() failed. Return -ENOMEM instead.

Besides, calling cifs_iget() here looks wrong as @fattr is not fully
set, yet. Why can't you use @inode from cifs_get_fattr() or do above
check right after cifs_get_fattr() in cifs_get_inode_info{,_unix}()?