Re: [MAINTAINER SUMMIT] Enforcing API deprecation

[Al Viro <viro@zeniv.linux.org.uk>]

On Tue, Aug 26, 2025 at 03:44:13PM -0700, Linus Torvalds wrote:

> If you can't automate a good 1:1 conversion that is clearly a no-op,
> maybe your new API is just complete garbage.
> 
> Honestly, if a new API is not a 100% obvious proper superset of the
> old one, WTF are you doing?
> 
> IOW, if you are creating a new API that doesn't express the EXACT SAME
> behavior as the old API did as one part of it, I think your new API is
> likely complete and utter shite.

Depends... An example of a bad API: dma_buf_fd().  It grabs a descriptor and
attaches an opened file associated with dmabuf to it;  lifetime of dmabuf
is that of the file in question, so the reference is consumed on success
and left to caller on failure.  Users (and that's after fixing a bunch of
broken ones during the last couple of cycles):

dma_heap_buffer_alloc():
	...
        fd = dma_buf_fd(dmabuf, fd_flags);
        if (fd < 0) {
                dma_buf_put(dmabuf);
                /* just return, as put will call release and that will free */
        }
        return fd;

udmabuf_create():
	...
        ret = dma_buf_fd(dmabuf, flags);
        if (ret < 0)
                dma_buf_put(dmabuf);

kfd_ioctl_export_dmabuf():
	...
        ret = dma_buf_fd(dmabuf, args->flags);
        if (ret < 0) {
                dma_buf_put(dmabuf);
                goto err_out;
        }
	...

intel_vgpu_get_dmabuf():
	...
        ret = dma_buf_fd(dmabuf, DRM_CLOEXEC | DRM_RDWR);
        if (ret < 0) {
                gvt_vgpu_err("create dma-buf fd failed ret:%d\n", ret);
                goto out_free_dmabuf;
        }
	...
out_free_dmabuf:
        dma_buf_put(dmabuf);

ttm_prime_handle_to_fd():
        ret = dma_buf_fd(dma_buf, flags);
        if (ret >= 0) {
                *prime_fd = ret;
                ret = 0;
        } else
                dma_buf_put(dma_buf);

vb2_core_expbuf():
        ret = dma_buf_fd(dbuf, flags & ~O_ACCMODE);
        if (ret < 0) {
                dprintk(q, 3, "buffer %d, plane %d failed to export (%d)\n",
                        vb->index, plane, ret);
                dma_buf_put(dbuf);
                return ret;
        }

fastrpc_dmabuf_alloc():
        bp.fd = dma_buf_fd(buf->dmabuf, O_ACCMODE);
        if (bp.fd < 0) {
                dma_buf_put(buf->dmabuf);
                return -EINVAL;
        }

... and in samples/video-mdev/mbochs.c:mbochs_get_gfx_dmabuf()
	...
        return dma_buf_fd(dmabuf->buf, 0);

Which leaks on failure, and samples/* is not a good place for that,
since it invites copying the leak back into drivers.

IMO that thing has wrong calling conventions.  I have not done anything
besides fixing the obvious bugs in users (and leaks were not even close
to the worst problems - try to access dmabuf after _success_ and you
get a UAF since another thread can guess the descriptor and close(2)
it) since I'm not involved in any of the areas where it's used, and when
asking opinions re changing the calling conventions got zero reaction...
*shrug*

I still think that saner replacement should consume the reference passed
to it both on success and on failure.  Theoretically, you can express
the original through that (bump refcount of file associated with dmabuf,
call that "saner replacement", then do dma_buf_put() only in case
of success), but at that point I'd say that original API is shite and
simulating it is just plain wrong.  _If_ we go for eliminating it,
that is - another approach is to grep through the tree once in a while
and watch out of new bugs cropping up in the callers.  Up to the
folks maintaining that area, really...

A similar thing around fs/* would have all in-tree users replaced, with
description placed in Documentation/filesystems/porting.rst ("recommended:
if you are using sucky_API(), see if you have run afoul of such and such
scenario.  Convert to replacement_API() in such and such way", basically)
and either taken out immediately (with s/recommended/mandatory/) or left
alone for a cycle or two, then ripped out.