Project

General

Profile

Actions
Copy link

Bug #5360

closed

Race condition in devfs upgrades reader to writer incidentally and causes panic

Added by Alexander Kolbasov over 8 years ago. Updated over 8 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
-
Start date:
2014-11-19
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
needs-triage
Gerrit CR:
External Bug:

Description

I saw a bug on DEBUG build with:

panic[cpu31]/thread=ffffff49cf894020: 
assertion failed: RW_READ_HELD(&ddv->sdev_contents), file: ../../common/fs/dev/sdev_subr.c, line: 2314

ffffff01edefcb70 genunix:process_type+162d40 ()
ffffff01edefcce0 dev:devname_readdir_func+220 ()
ffffff01edefcd50 dev:sdev_readdir+9a ()
ffffff01edefcde0 genunix:fop_readdir+6b ()
ffffff01edefcea0 genunix:getdents64+f5 ()
ffffff01edefcf00 unix:brand_sys_sysenter+2c1 ()

Here is how this can happen:
We enter prof_filldir() with reader lock.
Execute this:

    if (firsttime && rw_tryupgrade(&ddv->sdev_contents) == 0) {
        rw_exit(&ddv->sdev_contents);
        firsttime = 0;
        rw_enter(&ddv->sdev_contents, RW_WRITER);
        goto check_build;
    }

So we dropped reader lock and now have writer lock. Now we ho to check_build:

check_build:
    if ((ddv->sdev_flags & SDEV_BUILD) == 0 &&
        ddv->sdev_devtree_gen == devtree_gen &&
        (gdir == NULL || ddv->sdev_ldir_gen
        == gdir->sdev_gdir_gen))
        return;        /* already up to date */

If the condition satisfies this time, we exit with writer lock held which is wrong - we entered with reader lock only. The same issue is with the next line:

    /* We may have become a zombie (across a try) */
    if (ddv->sdev_state == SDEV_ZOMBIE)
        return;

Actions
Copy link
 #1

Updated by Alexander Kolbasov over 8 years ago

Here is a fix that actually works:

diff --git a/usr/src/uts/common/fs/dev/sdev_profile.c b/usr/src/uts/common/fs/dev/sdev_profile.c
index 65890b1..7d71fb1 100644
--- a/usr/src/uts/common/fs/dev/sdev_profile.c
+++ b/usr/src/uts/common/fs/dev/sdev_profile.c
@@ -655,51 +655,83 @@ prof_make_names(struct sdev_node *dir)
 }

 /*
+ * Return True if directory cache is out of date and should be updated.
+ */
+static boolean_t
+prof_dev_needupdate(sdev_node_t *ddv)
+{
+       sdev_node_t *gdir = ddv->sdev_origin;
+
+       /*
+        * Caller can have either reader or writer lock
+        */
+       ASSERT(RW_LOCK_HELD(&ddv->sdev_contents));
+
+       /*
+        * We need to rebuild the directory content if
+        * - ddv is not in a SDEV_ZOMBIE state
+        * - SDEV_BUILD is set OR
+        * - The device tree generation number has changed OR
+        * - The corresponding /dev namespace has been updated
+        */
+       return ((ddv->sdev_state != SDEV_ZOMBIE) &&
+               (((ddv->sdev_flags & SDEV_BUILD) != 0) ||
+               (ddv->sdev_devtree_gen != devtree_gen) ||
+               ((gdir != NULL) &&
+               (ddv->sdev_ldir_gen != gdir->sdev_gdir_gen))));
+}
+
+/*
  * Build directory vnodes based on the profile and the global
  * dev instance.
  */
 void
-prof_filldir(struct sdev_node *ddv)
+prof_filldir(sdev_node_t *ddv)
 {
-       int firsttime = 1;
-       struct sdev_node *gdir = ddv->sdev_origin;
+       sdev_node_t *gdir;

        ASSERT(RW_READ_HELD(&ddv->sdev_contents));

+       if (!prof_dev_needupdate(ddv)) {
+               ASSERT(RW_READ_HELD(&ddv->sdev_contents));
+               return;
+       }
        /*
-        * We need to rebuild the directory content if
-        * - SDEV_BUILD is set
-        * - The device tree generation number has changed
-        * - The corresponding /dev namespace has been updated
+        * Upgrade to writer lock
         */
-check_build:
-       if ((ddv->sdev_flags & SDEV_BUILD) == 0 &&
-           ddv->sdev_devtree_gen == devtree_gen &&
-           (gdir == NULL || ddv->sdev_ldir_gen
-           == gdir->sdev_gdir_gen))
-               return;         /* already up to date */
-
-       /* We may have become a zombie (across a try) */
-       if (ddv->sdev_state == SDEV_ZOMBIE)
-               return;
-
-       if (firsttime && rw_tryupgrade(&ddv->sdev_contents) == 0) {
+       if (rw_tryupgrade(&ddv->sdev_contents) == 0) {

        prof_make_symlinks(ddv);
Actions
Copy link
 #2

Updated by Electric Monk over 8 years ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 22253b45e469decdb988b799c90598f2652597cd

commit  22253b45e469decdb988b799c90598f2652597cd
Author: Alexander Kolbasov <akolb@sonic.net>
Date:   2015-01-20T23:26:20.000Z

    5360 Race condition in devfs upgrades reader to writer incidentally and causes panic
    Reviewed by: Robert Mustacchi <rm@joyent.com>
    Reviewed by: Dan McDonald <danmcd@omniti.com>
    Approved by: Richard Lowe <richlowe@richlowe.net>

Actions
Copy link

Also available in: Atom PDF