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

[Stefan Hajnoczi]

On Thu, Feb 04, 2021 at 09:25:28AM +0100, Greg Kurz wrote:
> 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()...

APIs should be designed to eliminate classes of errors where possible
IMO. Taking care regarding the uninitialized pointer in the error case
could be the caller's responsibility, but what's the advantage?

(There's a related thing with lo_inode_put(&inode) where it sets *inode
= NULL to eliminate use-after-free bugs in callers. It would have been
possible to use the same approach as free(3) where it's the caller's
responsiblity, but that API design decision in free(3) has caused
many bugs in applications.)

Stefan

signature.asc
Description: PGP signature