NetBSD-Bugs archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

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

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>
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)

  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


    | The possible fix of this bug is to replace "BUS_DMA_WAITOK" with

  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<>  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?


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.

Jia-Ju Bai

Home | Main Index | Thread Index | Old Index