Re: [PATCH] dma-buf: don't WARN_ON_ONCE() on a failing dmabuf->ops->vmap()

[Janghyuck Kim <janghyuck.kim@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 4951 bytes --]

On 2026-09-21 13:39, Christian König wrote:
> On 9/21/26 12:09, Janghyuck Kim wrote:
> > dma_buf_vmap() escalates any non-zero return from dmabuf->ops->vmap()
> > into a WARN_ON_ONCE(). But a failing vmap() is not, by itself,
> > evidence of a violated invariant or a driver bug -- it's an ordinary
> > runtime failure of a fallible operation, no different from any other
> > allocation-shaped kernel API that can return an error. The dma-buf
> > core has no visibility into what a given exporter's vmap()
> > implementation considers a normal failure versus an internal bug;
> > that distinction, if it matters at all, belongs to the exporter, not
> > to this generic passthrough.
> 
> Well not quite, apart from some not so relevant HW workarounds the vmap callback is mostly used for fbdev emulation.
> 
Fair point about fbdev being the historical motivation. However,
->vmap() is not limited to fbdev; for example, the mainline CMA dma-buf
heap calls vmap() and explicitly returns -ENOMEM when the mapping cannot
be established.

More generally, ->vmap() is a fallible callback whose API allows it to
return an error. The exact reason for failure is exporter-specific, so I
don't think the dma-buf core should treat every ->vmap() failure as an
exporter bug and unconditionally WARN on it.

The fatal_signal_pending() case is simply one concrete example of how
vmap() can fail under memory pressure without indicating a driver bug.
> > -ENOMEM is a concrete, easily reproduced example of this. Page
> > allocation on the vmap()/vmalloc() path can fail purely because the
> > calling task has a fatal signal pending: should_reclaim_retry() in
> > mm/page_alloc.c intentionally gives up reclaim and returns false
> > once fatal_signal_pending(current) is true, so that a task that is
> > already being killed (e.g. by a userspace low-memory killer) isn't
> > stuck retrying reclaim on its way out. The resulting -ENOMEM from
> > dmabuf->ops->vmap() is expected behaviour in that case, not a bug in
> > the exporter -- and it is far from the only way ops->vmap() can
> > legitimately fail (an exporter may just as well refuse to vmap a
> > buffer for other, unrelated reasons of its own).
> > 
> > This is exactly the kind of condition Documentation/process/coding-style.rst
> > warns against for WARN*():
> > 
> >     WARN*() must not be used for a condition that is expected to
> >     trigger easily, for example, by user space actions.
> > 
> > A failing vmap() is reachable purely by driving the system into low
> > memory and then issuing an ioctl that happens to map a dma-buf -- no
> > driver misbehaviour required. Turning that into a WARN_ON_ONCE(),
> > and on panic_on_warn kernels into an outright panic, doesn't help
> > debugging; it just turns an otherwise-recoverable error into a fatal
> > one.
> 
> WOW STOP! A driver allowing to vmap() through an IOCTL is a major no-go!
> 
> vmap() on a DMA-buf has very very limited use cases which should mostly only be trigger able as superuser.
> 
> At least on 32bit system you can otherwise trivially exhaust the vmap area of the kernel and that is even worse than a simple WARN_ON_ONCE().
Fair point. To be clear, I'm not arguing that an unprivileged ioctl
should be allowed to trigger vmap() without appropriate access control.
If a driver exposes that, I agree that it should be addressed in the
driver.

My point is separate: even a legitimate, one-off vmap() may fail under
memory pressure, and such a failure does not by itself indicate driver
misbehaviour. The fatal_signal_pending() case is one example where
memory allocation may give up rather than continue reclaim.

I'll drop the "no driver misbehaviour required" wording, since I agree
it could be read as endorsing unrestricted ioctl-triggered vmap, which
wasn't my intention.
> 
> Regards,
> Christian.
> 
> > 
> > Drop the WARN_ON_ONCE() and simply propagate the error, consistent
> > with how other allocation-failure paths in the kernel are handled.
> > The dma_buf_vmap_unlocked()/dma_buf_vmap() contract is unchanged:
> > both still return the negative errno from dmabuf->ops->vmap() to the
> > caller. Only the WARN_ON_ONCE() splat (and, transitively, the panic
> > on panic_on_warn kernels) is removed.
> > 
> > Signed-off-by: Janghyuck Kim <janghyuck.kim@samsung.com>
> > ---
> >  drivers/dma-buf/dma-buf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > index 36cdc342627e..252495761a21 100644
> > --- a/drivers/dma-buf/dma-buf.c
> > +++ b/drivers/dma-buf/dma-buf.c
> > @@ -1670,7 +1670,7 @@ int dma_buf_vmap(struct dma_buf *dmabuf, struct iosys_map *map)
> >         BUG_ON(iosys_map_is_set(&dmabuf->vmap_ptr));
> > 
> >         ret = dmabuf->ops->vmap(dmabuf, &ptr);
> > -       if (WARN_ON_ONCE(ret))
> > +       if (ret)
> >                 return ret;
> > 
> >         dmabuf->vmap_ptr = ptr;
> > --
> > 2.34.1
> > 
> 
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]