> > +config INFINIBAND_DMABUF
> > + bool "InfiniBand dma-buf support"
> > + depends on INFINIBAND_USER_MEM
>
> select ..some kind of DMABUF symbol...
Good catch. Will add the dependency.
>
> > + default n
> > + help
> > + Support for dma-buf based user memory.
> > + This allows userspace processes register memory regions
> > + backed by device memory exported as dma-buf, and thus
> > + enables RDMA operations using device memory.
>
> See remarks on the cover letter
Thanks.
> > +struct ib_umem *ib_umem_dmabuf_get(struct ib_device *device,
> > + unsigned long addr, size_t size,
> > + int dmabuf_fd, int access)
> > +{
> > + struct ib_umem_dmabuf *umem_dmabuf;
> > + struct sg_table *sgt;
> > + enum dma_data_direction dir;
> > + long ret;
> > +
> > + if (((addr + size) < addr) ||
> > + PAGE_ALIGN(addr + size) < (addr + size))
> > + return ERR_PTR(-EINVAL);
>
> This math validating the user parameters can overflow in various bad ways
This is modeled after the original ib_umem_get() function. Let me see how it can be done better.
>
> > + if (!can_do_mlock())
> > + return ERR_PTR(-EPERM);
>
> Why?
Hmm. Probably not needed.
>
> > + umem_dmabuf->umem.ibdev = device;
> > + umem_dmabuf->umem.length = size;
> > + umem_dmabuf->umem.address = addr;
> > + umem_dmabuf->umem.writable = ib_access_writable(access);
> > + umem_dmabuf->umem.is_dmabuf = 1;
> > + umem_dmabuf->umem.owning_mm = current->mm;
>
> Why does this need to store owning_mm?
For the mmdrop() call. But again, the mmgrab() and mmdrop() calls are probably not needed.
>
> > + umem_dmabuf->fd = dmabuf_fd;
>
> Doesn't need to store fd
>
You are right.
> > +void ib_umem_dmabuf_release(struct ib_umem_dmabuf *umem_dmabuf) {
> > + enum dma_data_direction dir;
> > +
> > + dir = umem_dmabuf->umem.writable ? DMA_BIDIRECTIONAL :
> > +DMA_FROM_DEVICE;
> > +
> > + /*
> > + * Only use the original sgt returned from dma_buf_map_attachment(),
> > + * otherwise the scatterlist may be freed twice due to the map caching
> > + * mechanism.
> > + */
> > + dma_buf_unmap_attachment(umem_dmabuf->attach, umem_dmabuf->sgt, dir);
> > + dma_buf_detach(umem_dmabuf->dmabuf, umem_dmabuf->attach);
> > + dma_buf_put(umem_dmabuf->dmabuf);
> > + mmdrop(umem_dmabuf->umem.owning_mm);
> > + kfree(umem_dmabuf);
> > +}
> > +EXPORT_SYMBOL(ib_umem_dmabuf_release);
>
> Why is this an EXPORT_SYMBOL?
It is called from ib_umem_release() which is in a different file.
>
> > diff --git a/include/rdma/ib_umem_dmabuf.h
> > b/include/rdma/ib_umem_dmabuf.h new file mode 100644 index
> > 0000000..e82b205
> > +++ b/include/rdma/ib_umem_dmabuf.h
>
> This should not be a public header
>
It's put there to be consistent with similar headers such as "ib_umem_odp.h". Can be changed.
> > @@ -0,0 +1,50 @@
> > +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
> > +/*
> > + * Copyright (c) 2020 Intel Corporation. All rights reserved.
> > + */
> > +
> > +#ifndef IB_UMEM_DMABUF_H
> > +#define IB_UMEM_DMABUF_H
> > +
> > +#include <linux/dma-buf.h>
> > +#include <rdma/ib_umem.h>
> > +#include <rdma/ib_verbs.h>
> > +
> > +struct ib_umem_dmabuf {
> > + struct ib_umem umem;
> > + int fd;
> > + struct dma_buf *dmabuf;
> > + struct dma_buf_attachment *attach;
> > + struct sg_table *sgt;
>
> Probably the ib_umem should be changed to hold a struct sg_table. Not clear to me why dma_buf wants to allocate this as a pointer..
ib_umem does hold a structure sg_table. The pointer field here is needed to avoid double free when the dma-buf is unmapped and detached. The internal caching mechanism of dma-buf checks the value of sgt itself to determine if a sg table needs to be freed at the time of unmapping or detaching. Passing the address of the sg table structure of ib_umem would lead to double free.
>
> Also this can be in umem_dmabuf.c
>
> > +static inline struct ib_umem_dmabuf *to_ib_umem_dmabuf(struct ib_umem
> > +*umem) {
> > + return container_of(umem, struct ib_umem_dmabuf, umem); }
>
> Put in ummem_dmabuf.c
Will do.
>
> > +
> > +#ifdef CONFIG_INFINIBAND_DMABUF
> > +
> > +struct ib_umem *ib_umem_dmabuf_get(struct ib_device *device,
> > + unsigned long addr, size_t size,
> > + int dmabuf_fd, int access);
> > +
> > +void ib_umem_dmabuf_release(struct ib_umem_dmabuf *umem_dmabuf);
> > +
> > +#else /* CONFIG_INFINIBAND_DMABUF */
> > +
> > +static inline struct ib_umem *ib_umem_dmabuf_get(struct ib_device *device,
> > + unsigned long addr,
> > + size_t size, int dmabuf_fd,
> > + int access)
> > +{
> > + return ERR_PTR(-EINVAL);
> > +}
>
> This should be in the existing ib_umem.h
>
Good. Thanks.
> > +
> > +static inline void ib_umem_dmabuf_release(struct ib_umem_dmabuf
> > +*umem_dmabuf) { }
>
> In uverbs_priv.h
>
Will do.
> Jason
