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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAFTVevUNkkputV3SWJzh8D4Fw3b9e_fGHb=R-Mk+C-QrhC1uQg@mail.gmail.com>
Date: Wed, 13 Mar 2024 09:47:54 +0530
From: Meetakshi Setiya <meetakshisetiyaoss@...il.com>
To: Paulo Alcantara <pc@...guebit.com>
Cc: sfrench@...ba.org, 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

Hi

PFA the follow up patch after the above revision.

Thanks
Meetakshi

On Wed, Mar 6, 2024 at 8:01 PM Paulo Alcantara <pc@...guebit.com> wrote:
>
> meetakshisetiyaoss@...il.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}()?

Download attachment "0002-smb-client-do-not-defer-close-open-handles-to-delete.patch" of type "application/octet-stream" (8982 bytes)