Re: kern/52331: ydc driver: sleep-under-spin-mutex bugs in yds_allocmem

[Jia-Ju Bai <baijiaju1990%163.com@localhost>]

The following reply was made to PR kern/52331; it has been noted by GNATS.

From: Jia-Ju Bai <baijiaju1990%163.com@localhost>
To: Robert Elz <kre%munnari.OZ.AU@localhost>
Cc: gnats-bugs%NetBSD.org@localhost, kern-bug-people%netbsd.org@localhost, 
 gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
Subject: Re: kern/52331: ydc driver: sleep-under-spin-mutex bugs in yds_allocmem
Date: Sun, 25 Jun 2017 21:14:19 +0800

 On 06/25/2017 08:55 PM, Robert Elz wrote:
 > The following reply was made to PR kern/52331; it has been noted by GNATS.
 >
 > From: Robert Elz<kre%munnari.OZ.AU@localhost>
 > To: gnats-bugs%NetBSD.org@localhost
 > Cc: nat%netbsd.org@localhost
 > Subject: Re: kern/52331: ydc driver: sleep-under-spin-mutex bugs in yds_allocmem
 > Date: Sun, 25 Jun 2017 19:53:31 +0700
 >
 >       Date:        Sun, 25 Jun 2017 08:45:00 +0000 (UTC)
 >       From:        baijiaju1990%163.com@localhost
 >       Message-ID:<20170625084500.68FE47A2B0%mollari.NetBSD.org@localhost>
 >
 >   While your analysis tool seems good at finding code worth reviewing, I
 >   am not sure your review of the code to determine if there is a bug or
 >   not in this case is quite up to it.
 >
 >     | The driver may sleep in interrupt, and the function call path in file "sys/dev/pci/yds.c" in NetBSD-7.1 release is:
 >     | yds_resume [acquire the spin mutex]
 >     |   yds_init
 >     |     yds_allocate_slots
 >     |       yds_allocmem
 >     |         bus_dmamem_alloc(BUS_DMA_WAITOK) -->  may sleep
 >     |         bus_dmamem_map(BUS_DMA_WAITOK) -->  may sleep
 >     |         bus_dmamem_create(BUS_DMA_WAITOK) -->  may sleep
 >     |         bus_dmamem_load(BUS_DMA_WAITOK) -->  may sleep
 >     |
 >
 >   First:
 >
 >     | The possible fix of this bug is to replace "BUS_DMA_WAITOK" with
 >     | "BUS_DMA_NOWAIT".
 >
 >   while that would avoid a potential sleep it would not actually work (if
 >   the sleep was ever necessary) as then the resources would not be allocated.
 >
 >   When yds_resume() calls yds_init() the driver must have already been
 >   initialised, yds_init() is first called in yds_attach(), and if
 >   it fails, the attach also fails - in that case the code never
 >   reaches the code (right at the end of yds_attach() which
 >   establishes yds_resume as the "switch back on" power handler.
 >
 >   So yds_resume() cannot be called unless yds_init() has succeeded.
 >
 >   One of the events that makes yds_init() fail is if yds_allocate_slots() fails.
 >
 >   yds_allocate_slots() only calls yds_allocmem() if KERNADDR(p) is NULL,
 >   where p =&sc->sc_ctrldata; (KERNADDR is p->addr)
 >
 >   If that happens, that is, if yds_allocmem() is called, yds_allocate_slots()
 >   fails if yds_allocmem() fails - once again, if that happens in the call
 >   that comes from yds_init() from yds_attach() the attach fails, and yds_resume
 >   can never be called.
 >
 >   yds_allocmem() does call bus_dmamem_alloc() (etc) as your PR revealed,
 >   but remember is only called if p->addr == NULL.
 >
 >   The second bus_dma*() call in yds_allocmem() is
 >
 >   	        error = bus_dmamem_map(sc->sc_dmatag, p->segs, p->nsegs, p->size,
 >                                  &p->addr, BUS_DMA_WAITOK|BUS_DMA_COHERENT);
 >
 >   That sets p_addr (unless it fails, in which case yds_allocmem returns the
 >   resources it has already claimed, and fails, and when that happens,
 >   yds_allocate_slots() also fails, which causes yds_init() to fail, which
 >   causes yds_attach() to fail, and yds_resume can never be called.
 >
 >   So we know that for yds_resume to be called, the yds_init() in yds_attach()
 >   must have succeeded, which means that yds_allocate_slots() succeeded, which
 >   means that yds_allocmem() succeeded, which means that p->addr != NULL when
 >   yds_attach() is finished with the yds_init() call.
 >
 >   Any later call of yds_allocmem() will find p->addr != NULL, and never call
 >   yds_allocmem() again (or not until yds_freemem() called from yds_free() has
 >   returned it all - that is only called from audio.c, but it wull take someone
 >   more familiar with the code than I can ever be to know whether that is
 >   possible in a situation where the power management resume function might
 >   still later be called.)
 >
 >   But what's more, when yds_freemem() actually releases the resources
 >   identified by p_addr (and the others allocated by yds_allocmem()) it
 >   never bothers to set the pointer(s) back to NULL, so even if it were
 >   possible that yds_free() might be called from audio.c, and the power
 >   handler resume function called later, I still don't see how yds_allocmem()
 >   can ever be called again.
 >
 >   I have cc'd Nathanial Sloss<nat%netbsd.org@localhost>  on this reply - Nat, do you
 >   want this PR, or can we just assume that the bug reported is not in fact
 >   possible, and close it?
 >
 >   kre
 >
 
 Thanks for your reply and detailed analysis :)
 
  From your words, I can see that "if (KERNADDR(p) == NULL)" is always 
 not satisfied at runtime, is it?
 If it is, I think it is okay to remove the code (including yds_allocmem) 
 of this if condition.
 
 Thanks,
 Jia-Ju Bai