Re: [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 3101 bytes --]

On Wed, 3 Feb 2021 17:00:06 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Wed, Feb 03, 2021 at 03:20:14PM +0100, Greg Kurz wrote:
> > On Wed,  3 Feb 2021 11:37:18 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > 
> > > lo_do_lookup() finds an existing inode or allocates a new one. It
> > > increments nlookup so that the inode stays alive until the client
> > > releases it.
> > > 
> > > Existing callers don't need the struct lo_inode so the function doesn't
> > > return it. Extend the function to optionally return the inode. The next
> > > commit will need it.
> > > 
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++--------
> > >  1 file changed, 21 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index e63cbd3fb7..c87a1f3d72 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
> > >  }
> > >  
> > >  /*
> > > - * Increments nlookup and caller must release refcount using
> > > - * lo_inode_put(&parent).
> > > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be
> > > + * called eventually to decrement nlookup again. If inodep is non-NULL, the
> > > + * inode pointer is stored and the caller must call lo_inode_put().
> > >   */
> > >  static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > -                        struct fuse_entry_param *e)
> > > +                        struct fuse_entry_param *e,
> > > +                        struct lo_inode **inodep)
> > >  {
> > >      int newfd;
> > >      int res;
> > > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > >      struct lo_inode *inode = NULL;
> > >      struct lo_inode *dir = lo_inode(req, parent);
> > >  
> > > +    if (inodep) {
> > > +        *inodep = NULL;
> > > +    }
> > > +
> > 
> > Is this side-effect needed ? If lo_do_lookup() returns an error, it
> > rather seems that the caller shouldn't expect anything to be written
> > here, i.e. the content of *inodep still belongs to the caller and
> > whatever value it previously put in there (as patch 3/3 does) should
> > be preserved IMHO.
> > 
> > Apart from that LGTM.
> 
> I like this approach because it prevents accessing uninitialized memory
> in the caller:
> 
>   struct lo_inode *inode;
> 
>   if (lo_do_lookup(..., &inodep) != 0) {
>     goto err;
>   }
>   ...
> 
>   err:
>   lo_inode_put(&inode); <-- uninitialized in the error case!

My point is that it is the caller's business to ensure that inode
doesn't contain garbage if it is to be used irrespective of the
outcome of lo_do_lookup(). This is precisely what patch 3/3 does,
so I don't understand the ultimate purpose of nullifying the
inode pointer _again_ in lo_do_lookup()...

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]