Received-SPF: pass (google.com: domain of linux-kernel+bounces-94056-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <8e729f88f285fefaa8a089da09484ed2@manguebit.com>
From: Paulo Alcantara <pc@manguebit.com>
To: meetakshisetiyaoss@gmail.com, sfrench@samba.org,
 ronniesahlberg@gmail.com, sprasad@microsoft.com, nspmangalore@gmail.com,
 tom@talpey.com, linux-cifs@vger.kernel.org, linux-kernel@vger.kernel.org,
 samba-technical@lists.samba.org, bharathsm.hsk@gmail.com
Cc: Meetakshi Setiya <msetiya@microsoft.com>
Subject: Re: [PATCH 2/3] smb: client: do not defer close open handles to
 deleted files
In-Reply-To: <20240306034353.190039-2-meetakshisetiyaoss@gmail.com>
References: <20240306034353.190039-1-meetakshisetiyaoss@gmail.com>
 <20240306034353.190039-2-meetakshisetiyaoss@gmail.com>
Date: Wed, 06 Mar 2024 11:31:48 -0300
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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}()?