Hi
PFA the follow up patch after the above revision.
Thanks
Meetakshi
On Wed, Mar 6, 2024 at 8:01 PM Paulo Alcantara <pc@xxxxxxxxxxxxx> wrote:
>
> meetakshisetiyaoss@xxxxxxxxx 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}()?
Attachment:
0002-smb-client-do-not-defer-close-open-handles-to-delete.patch
Description: Binary data
