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 <[email protected]> > > --- > > 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 > > > >
