Bug 219988

Summary: [kernel] sys/kern/kern_sysctl.c:a sleep-under-rmlock bug in sysctl_add_oid
Product: Base System Reporter: Jia-Ju Bai <baijiaju1990>
Component: kernAssignee: freebsd-bugs (Nobody) <bugs>
Status: Closed Not A Bug    
Severity: Affects Only Me CC: baijiaju1990, cem, emaste, markj, mjg
Priority: --- Keywords: patch
Version: CURRENT   
Hardware: Any   
OS: Any   

Description Jia-Ju Bai 2017-06-14 14:30:32 UTC
The kernel may sleep under a rmlock, and the function call path in file "sys/kern/kern_sysctl.c" in FreeBSD 11.0 is:
sysctl_add_oid [line 706: acquire the rmlock]
  malloc(M_WAITOK) [line 722] --> may sleep

The possible fix of this bug is to replace "M_WAITOK" with "M_NOWAIT" in malloc.

This bug is found by a static analysis tool written by myself, and it is checked by my review of the FreeBSD code.

Jia-Ju Bai
Comment 1 Conrad Meyer freebsd_committer 2017-06-14 15:17:36 UTC
I suggest moving the allocation out from under the lock rather than converting an M_WAITOK allocation to M_NOWAIT.  E.g.,

--- a/sys/kern/kern_sysctl.c
+++ b/sys/kern/kern_sysctl.c
@@ -698,11 +698,12 @@ sysctl_add_oid(struct sysctl_ctx_list *clist, struct sysctl_oid_list *parent,
        int (*handler)(SYSCTL_HANDLER_ARGS), const char *fmt, const char *descr,
        const char *label)
 {
-       struct sysctl_oid *oidp;
+       struct sysctl_oid *oidp, *storage;

        /* You have to hook up somewhere.. */
        if (parent == NULL)
                return(NULL);
+       storage = malloc(sizeof(*storage), M_SYSCTLOID, M_WAITOK|M_ZERO);
        /* Check if the node already exists, otherwise create it */
        SYSCTL_WLOCK();
        oidp = sysctl_find_oidname(name, parent);
@@ -713,14 +714,16 @@ sysctl_add_oid(struct sysctl_ctx_list *clist, struct sysctl_oid_list *parent,
                        if (clist != NULL)
                                sysctl_ctx_entry_add(clist, oidp);
                        SYSCTL_WUNLOCK();
+                       free(storage);
                        return (oidp);
                } else {
                        SYSCTL_WUNLOCK();
                        printf("can't re-use a leaf (%s)!\n", name);
+                       free(storage);
                        return (NULL);
                }
        }
-       oidp = malloc(sizeof(struct sysctl_oid), M_SYSCTLOID, M_WAITOK|M_ZERO);
+       oidp = storage;
        oidp->oid_parent = parent;
        SLIST_INIT(&oidp->oid_children);
        oidp->oid_number = number;
Comment 2 Mateusz Guzik freebsd_committer 2017-06-14 16:16:00 UTC
The lock at hand is initialized with RM_SLEEPABLE, i.e. sleep is permitted.

It probably is desirable to turn it into a non-sleepable lock, but I don't think it's a high priority (and right now I don't even if this is feasible).

The patch at hand is definitely incomplete even for sysctl_add_oid. strdup also performs M_WAITOK.
Comment 3 Conrad Meyer freebsd_committer 2017-06-14 16:44:52 UTC
(In reply to Mateusz Guzik from comment #2)
Sure.  But the same mechanism can be used there too.
Comment 4 Mark Johnston freebsd_committer 2019-05-13 17:02:50 UTC
Do I understand correctly that the report is not a bug?  As noted, the rmlock write lock is sleepable, so M_WAITOK allocations are permitted if undesirable.
Comment 5 Conrad Meyer freebsd_committer 2019-05-13 17:25:53 UTC
I believe you are correct — this specific malloc-under-lock is not a locking bug.

That said, I think it might be desirable to eventually revamp some of the locking around sysctl.  I don't think we need to keep this PR open to track that.
Comment 6 Mark Johnston freebsd_committer 2019-05-13 17:31:49 UTC
(In reply to Conrad Meyer from comment #5)
Thanks, I agree.