- Download Diff

Summary:

8805 xattr_dir_lookup() can leak a vnode hold

Review Request #788 — Created Dec. 4, 2017 and submitted June 21, 2018, 2:25 p.m.

Information
Owner:	vgusev
Repository:	illumos-gate
Branch:	master
Bugs:	8805
Depends On:
Commit:	d15e368...
Reviewers
Groups:	general
People:	gwr

Description

8805 xattr_dir_lookup() can leak a vnode hold

Testing Done

After steps described at #8805, dataset is umounted successfully, i.e. no "busy".

usr/src/uts/common/fs/xattr.c (Diff revision 1)

Can you please explain what's going on here?
How do we end up with an extra ref?

How do we know it's OK to call VN_RELE_LOCKD here?
(which is only OK if dvp->v_count > 1 right?
at least comparing with what vn_rele does)

usr/src/uts/common/fs/xattr.c (Diff revision 1)

Yes, sure.

Here is allocation and initialization with

pre-alloc-check
allocation
post-alloc-check.

dvp->v_xattrdir is checked first time on the line 1614 under held lock.
gfs_dir_create() is called w/o lock (i.e. allocation).
dvp->v_xattrdir is checked again and set on the line 1691 (orig) under held dvp->v_lock, i.e. post alloc check. And thic check introduces two paths: winner and looser.

As described in comments for case "dvp->v_xattr != NULL":

            /*
             * We lost the race to create the xattr dir.
             * Destroy this one, use the winner.  We can't
             * just call VN_RELE(*vpp), because the vnode
             * is only partially initialized.
             */

and the code below comment is doing manual freeing.

Note, dvp->v_count >= 2 before line 1674 (new) because:
a. winner has got the reference already in gfs_dir_create()
b. this check is under held lock
c. looser also got the reference in gfs_dir_create()

d. reference is released safely under held lock in gfs_file_release()

Note-2, winner is: lines 1690-1691 (orig)

usr/src/uts/common/fs/xattr.c (Diff revision 1)
```
In d. should be gfs_file_inactive()
```

Ship it!

```
Ship It!
```

usr/src/uts/common/fs/xattr.c (Diff revision 1)

Maybe minor nit:

Strictly speaking, this comment is not needed here and might be confusing a bit (we do not have similar comment at line 1615).

I suggest either to remove it, or replace it with something like:

/*
 * Use the winner
 */

But if you do not wish do to do, I'm okay with that.

marcel Dec. 5, 2017, 5:44 a.m.

Damn, I meant: But if you do not want to do so, ...

vgusev Dec. 5, 2017, 8:43 a.m.

Marcel, comment "dvp was held by winner" means additional VN_HOLD(dvp) is not required here, because dvp must be held only once in gfs_dir_create() for  the winner.
Note, winner path is:
  else {
            (*vpp)->v_flag |= (V_XATTRDIR|V_SYSATTR);
            dvp->v_xattrdir = *vpp;
  }

marcel Dec. 5, 2017, 8:50 a.m.
```
Yes, I know what that comment mean :-).
```

vgusev Dec. 5, 2017, 9:10 a.m.

Ok. Let me to put more explanation :)

Strictly speaking, this comment is not needed here and might be confusing a bit (we do not have similar comment at line 1615).

Line 1615 does have similar comment because it is not initialization of dvp->v_xattrdir. OTW, @dvp holding must be only done when dvp->v_xattrdir is set in order to have proper backward reference via v_xattrdir to parent.
I believe comment is required because previous comments were deleted and here is justification why previous comment was wrong.

vgusev Dec. 5, 2017, 9:22 a.m.
```
s/Line 1615 does not have/
```

marcel Dec. 5, 2017, 9:33 a.m.

I was afraid you won't understand what I mean. :-(
So as I said, feel free to ignore this minor nit.

But if you want to think about it a bit more then:

At lines 1682-1683 (those we are talking about here) the dvp->v_xattrdir is not set either (similarly as it is not set at lines 1615-1616).
It is usually not a good idea to explain in a comment what was wrong in a comment that is no longer there. Such information belongs somewhere else (into the bug report, for example). Comments should describe the current situation.

But please do not argue more about that. It is just a waste of our time. As I said: ship it.

usr/src/uts/common/fs/xattr.c (Diff revision 1)

At least for me, it would have helped a lot (when I last worked on this code, rather long ago) if there were a more helpful comment here about what vnode holds come from where in both the winner and loser code paths, and what happens to those holds (returned to caller, released).

vgusev Dec. 6, 2017, 7:20 a.m.

Gordon, I will add some comments to make it more evident.
This is commonly used approach to get/allocate resource out of lock. 
Standard approach is:

   mutex_lock();
   if (pdata) {
         data_get(pdata);  /* optional, if pdata is not freed */
         p = pdata;
         mutex_unlock();
         return p;
   }
   pdata = alloc();   /* could be quite slow and is executed during held mutex */
   mutex_unlock();


Out-of-lock  approach:

   /* fast check */
   mutex_lock()
   if (pdata) {
         data_get(pdata);  /* optional, if there is no implemented data_put() */
         p = pdata;
         mutex_unlock();
         return p;
   }
   mutex_unlock();


   /* slow path: allocation */
   p = alloc();

   /* post allocation check and set */
   mutex_lock();
   if (pdata != NULL) {
        /* lost the race */

        free(p);

        /* the same as in fast check */
        data_get(pdata);
        p = pdata;

   } else {
     /* winner */

     pdata = p;   /* generally alloc() set reference to 1, thus no need holding data here */
   }
   mutex_unlock();

   return p;

gwr Dec. 6, 2017, 4:43 p.m.

I'm familiar with that design pattern. I simply found it not obvious how that pattern was mapped to this implementation.
Maybe I just didn't look long enough. (I'm rather busy lately).

vgusev Dec. 6, 2017, 6:34 p.m.

dvp --> xattrdir -->
^_____|

dvp points to xattrdir-vp. xattrdir-vp has backward reference to its parent (dvp) with +1 refcount.
Lookups in dvp returns held xattrdir-vp.