Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

24 participants
2727 discussions

Re: [Linaro-mm-sig] [PATCH v2] dma-buf: acquire name lock before read/write dma_buf.name

by Christian König

Am 08.10.21 um 09:54 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Because dma-buf.name can be freed in func: "dma_buf_set_name", > so, we need to acquire lock first before we read/write dma_buf.name > to prevent Use After Free(UAF) issue. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> Going to push that upstream if nobody else objects. Thanks, Christian. > --- > drivers/dma-buf/dma-buf.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 511fe0d217a0..a7f6fd13a635 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1372,6 +1372,8 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > if (ret) > goto error_unlock; > > + > + spin_lock(&buf_obj->name_lock); > seq_printf(s, "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n", > buf_obj->size, > buf_obj->file->f_flags, buf_obj->file->f_mode, > @@ -1379,6 +1381,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > buf_obj->exp_name, > file_inode(buf_obj->file)->i_ino, > buf_obj->name ?: ""); > + spin_unlock(&buf_obj->name_lock); > > robj = buf_obj->resv; > fence = dma_resv_excl_fence(robj);

3 years, 6 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: acquire name lock before read/write dma_buf.name

by Christian König

Am 08.10.21 um 08:29 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Because dma-buf.name can be freed in func: "dma_buf_set_name", > so, we need to acquire lock first before we read/write dma_buf.name > to prevent Use After Free(UAF) issue. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/dma-buf.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 511fe0d217a0..aebb51b3ff52 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -80,7 +80,9 @@ static void dma_buf_release(struct dentry *dentry) > dma_resv_fini(dmabuf->resv); > > module_put(dmabuf->owner); > + spin_lock(&dmabuf->name_lock); > kfree(dmabuf->name); > + spin_unlock(&dmabuf->name_lock); That here is certainly a NAK. This is the release function if somebody is changing the name on a released DMA-buf we have much bigger problems. > kfree(dmabuf); > } > > @@ -1372,6 +1374,8 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > if (ret) > goto error_unlock; > > + > + spin_lock(&dmabuf->name_lock); > seq_printf(s, "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n", > buf_obj->size, > buf_obj->file->f_flags, buf_obj->file->f_mode, > @@ -1379,6 +1383,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > buf_obj->exp_name, > file_inode(buf_obj->file)->i_ino, > buf_obj->name ?: ""); > + spin_unlock(&dmabuf->name_lock); Yeah, that part looks like a good idea to me as well. Christian. > > robj = buf_obj->resv; > fence = dma_resv_excl_fence(robj);

3 years, 6 months

[PATCH 1/2] dma-buf: add dma_resv_for_each_fence v3

by Christian König

A simpler version of the iterator to be used when the dma_resv object is locked. v2: fix index check here as well v3: minor coding improvement, some documentation cleanup Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/dma-buf/dma-resv.c | 51 ++++++++++++++++++++++++++++++++++++++ include/linux/dma-resv.h | 20 +++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index a480af9581bd..2f98caa68ae5 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -423,6 +423,57 @@ struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor) } EXPORT_SYMBOL(dma_resv_iter_next_unlocked); +/** + * dma_resv_iter_first - first fence from a locked dma_resv object + * @cursor: cursor to record the current position + * + * Return the first fence in the dma_resv object while holding the + * &dma_resv.lock. + */ +struct dma_fence *dma_resv_iter_first(struct dma_resv_iter *cursor) +{ + struct dma_fence *fence; + + dma_resv_assert_held(cursor->obj); + + cursor->index = 0; + if (cursor->all_fences) + cursor->fences = dma_resv_shared_list(cursor->obj); + else + cursor->fences = NULL; + + fence = dma_resv_excl_fence(cursor->obj); + if (!fence) + fence = dma_resv_iter_next(cursor); + + cursor->is_restarted = true; + return fence; +} +EXPORT_SYMBOL_GPL(dma_resv_iter_first); + +/** + * dma_resv_iter_next - next fence from a locked dma_resv object + * @cursor: cursor to record the current position + * + * Return the next fences from the dma_resv object while holding the + * &dma_resv.lock. + */ +struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor) +{ + unsigned int idx; + + dma_resv_assert_held(cursor->obj); + + cursor->is_restarted = false; + if (!cursor->fences || cursor->index >= cursor->fences->shared_count) + return NULL; + + idx = cursor->index++; + return rcu_dereference_protected(cursor->fences->shared[idx], + dma_resv_held(cursor->obj)); +} +EXPORT_SYMBOL_GPL(dma_resv_iter_next); + /** * dma_resv_copy_fences - Copy all fences from src to dst. * @dst: the destination reservation object diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 764138ad8583..491359cea54c 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -179,6 +179,8 @@ struct dma_resv_iter { struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor); struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor); +struct dma_fence *dma_resv_iter_first(struct dma_resv_iter *cursor); +struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor); /** * dma_resv_iter_begin - initialize a dma_resv_iter object @@ -244,6 +246,24 @@ static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) for (fence = dma_resv_iter_first_unlocked(cursor); \ fence; fence = dma_resv_iter_next_unlocked(cursor)) +/** + * dma_resv_for_each_fence - fence iterator + * @cursor: a struct dma_resv_iter pointer + * @obj: a dma_resv object pointer + * @all_fences: true if all fences should be returned + * @fence: the current fence + * + * Iterate over the fences in a struct dma_resv object while holding the + * &dma_resv.lock. @all_fences controls if the shared fences are returned as + * well. The cursor initialisation is part of the iterator and the fence stays + * valid as long as the lock is held and so no extra reference to the fence is + * taken. + */ +#define dma_resv_for_each_fence(cursor, obj, all_fences, fence) \ + for (dma_resv_iter_begin(cursor, obj, all_fences), \ + fence = dma_resv_iter_first(cursor); fence; \ + fence = dma_resv_iter_next(cursor)) + #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) -- 2.25.1

3 years, 6 months

Deploying new iterator interface for dma-buf

by Christian König

Hi guys, I've fixed up the lockdep splat in the new selftests found by the CI systems and added another path for dma_resv_poll. I know you guys are flooded, but can we get at least the first few patches committed? The patches to change the individual drivers could also be pushed later on I think. Thanks, Christian.

3 years, 6 months

Re: [Linaro-mm-sig] [PATCH net-next 2/5] dt-bindings: net: brcm, unimac-mdio: Add asp-v2.0

by Rob Herring

On Fri, 24 Sep 2021 14:44:48 -0700, Justin Chen wrote: > The ASP 2.0 Ethernet controller uses a brcm unimac. > > Signed-off-by: Justin Chen <justinpopo6(a)gmail.com> > Signed-off-by: Florian Fainelli <f.fainelli(a)gmail.com> > --- > Documentation/devicetree/bindings/net/brcm,unimac-mdio.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring <robh(a)kernel.org>

3 years, 6 months

Re: [Linaro-mm-sig] [PATCH v6 2/2] habanalabs: add support for dma-buf exporter

by Jason Gunthorpe

On Thu, Sep 30, 2021 at 03:46:35PM +0300, Oded Gabbay wrote: > After reading the kernel iommu code, I think this is not relevant > here, and I'll add a comment appropriately but I'll also write it > here, and please correct me if my understanding is wrong. > > The memory behind this specific dma-buf has *always* resided on the > device itself, i.e. it lives only in the 'device' domain (after all, > it maps a PCI bar address which points to the device memory). > Therefore, it was never in the 'CPU' domain and hence, there is no > need to perform a sync of the memory to the CPU's cache, as it was > never inside that cache to begin with. > > This is not the same case as with regular memory which is dma-mapped > and then copied into the device using a dma engine. In that case, > the memory started in the 'CPU' domain and moved to the 'device' > domain. When it is unmapped it will indeed be recycled to be used > for another purpose and therefore we need to sync the CPU cache. > > Is my understanding correct ? It makes sense to me Jason

3 years, 6 months

Re: [Linaro-mm-sig] [PATCH v6 2/2] habanalabs: add support for dma-buf exporter

by Jason Gunthorpe

On Wed, Sep 29, 2021 at 12:17:35AM +0300, Oded Gabbay wrote: > On Tue, Sep 28, 2021 at 8:36 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > On Sun, Sep 12, 2021 at 07:53:09PM +0300, Oded Gabbay wrote: > > > From: Tomer Tayar <ttayar(a)habana.ai> > > > > > > Implement the calls to the dma-buf kernel api to create a dma-buf > > > object backed by FD. > > > > > > We block the option to mmap the DMA-BUF object because we don't support > > > DIRECT_IO and implicit P2P. > > > > This statement doesn't make sense, you can mmap your dmabuf if you > > like. All dmabuf mmaps are supposed to set the special bit/etc to > > exclude them from get_user_pages() anyhow - and since this is BAR > > memory not struct page memory this driver would be doing it anyhow. > > > But we block mmap the dmabuf fd from user-space. > If you try to do it, you will get MAP_FAILED. You do, I'm saying the above paragraph explaining *why* that was done is not correct. > > > We check the p2p distance using pci_p2pdma_distance_many() and refusing > > > to map dmabuf in case the distance doesn't allow p2p. > > > > Does this actually allow the p2p transfer for your intended use cases? > > It depends on the system. If we are working bare-metal, then yes, it allows. > If inside a VM, then no. The virtualized root complex is not > white-listed and the kernel can't know the distance. > But I remember you asked me to add this check, in v3 of the review IIRC. > I don't mind removing this check if you don't object. Yes, i tis the right code, I was curious how far along things have gotten > > Don't write to the kernel log from user space triggered actions > at all ? At all. > It's the first time I hear about this limitation... Oh? It is a security issue, we don't want to allow userspace to DOS the kerne logging. > How do you tell the user it has done something wrong ? dev_dbg is the usual way and then users doing debugging can opt in to the logging. > > Why doesn't this return a sg_table * and an ERR_PTR? > Basically I modeled this function after amdgpu_vram_mgr_alloc_sgt() > And in that function they also return int and pass the sg_table as ** > > If it's critical I can change. Please follow the normal kernel style Jason

3 years, 6 months

Re: [Linaro-mm-sig] [RFC PATCH v2 2/2] RDMA/rxe: Add dma-buf support

by Daniel Vetter

On Wed, Sep 29, 2021 at 01:19:05PM +0900, Shunsuke Mie wrote: > Implement a ib device operation ‘reg_user_mr_dmabuf’. Generate a > rxe_map from the memory space linked the passed dma-buf. > > Signed-off-by: Shunsuke Mie <mie(a)igel.co.jp> > --- > drivers/infiniband/sw/rxe/rxe_loc.h | 2 + > drivers/infiniband/sw/rxe/rxe_mr.c | 118 ++++++++++++++++++++++++++ > drivers/infiniband/sw/rxe/rxe_verbs.c | 34 ++++++++ > drivers/infiniband/sw/rxe/rxe_verbs.h | 2 + > 4 files changed, 156 insertions(+) > > diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h > index 1ca43b859d80..8bc19ea1a376 100644 > --- a/drivers/infiniband/sw/rxe/rxe_loc.h > +++ b/drivers/infiniband/sw/rxe/rxe_loc.h > @@ -75,6 +75,8 @@ u8 rxe_get_next_key(u32 last_key); > void rxe_mr_init_dma(struct rxe_pd *pd, int access, struct rxe_mr *mr); > int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, > int access, struct rxe_mr *mr); > +int rxe_mr_dmabuf_init_user(struct rxe_pd *pd, int fd, u64 start, u64 length, > + u64 iova, int access, struct rxe_mr *mr); > int rxe_mr_init_fast(struct rxe_pd *pd, int max_pages, struct rxe_mr *mr); > int rxe_mr_copy(struct rxe_mr *mr, u64 iova, void *addr, int length, > enum rxe_mr_copy_dir dir); > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c > index 53271df10e47..af6ef671c3a5 100644 > --- a/drivers/infiniband/sw/rxe/rxe_mr.c > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c > @@ -4,6 +4,7 @@ > * Copyright (c) 2015 System Fabric Works, Inc. All rights reserved. > */ > > +#include <linux/dma-buf.h> > #include "rxe.h" > #include "rxe_loc.h" > > @@ -245,6 +246,120 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, > return err; > } > > +static int rxe_map_dmabuf_mr(struct rxe_mr *mr, > + struct ib_umem_dmabuf *umem_dmabuf) > +{ > + struct rxe_map_set *set; > + struct rxe_phys_buf *buf = NULL; > + struct rxe_map **map; > + void *vaddr, *vaddr_end; > + int num_buf = 0; > + int err; > + size_t remain; > + > + mr->dmabuf_map = kzalloc(sizeof &mr->dmabuf_map, GFP_KERNEL); dmabuf_maps are just tagged pointers (and we could shrink them to actually just a tagged pointer if anyone cares about the overhead of the separate bool), allocating them seperately is overkill. > + if (!mr->dmabuf_map) { > + err = -ENOMEM; > + goto err_out; > + } > + > + err = dma_buf_vmap(umem_dmabuf->dmabuf, mr->dmabuf_map); > + if (err) > + goto err_free_dmabuf_map; > + > + set = mr->cur_map_set; > + set->page_shift = PAGE_SHIFT; > + set->page_mask = PAGE_SIZE - 1; > + > + map = set->map; > + buf = map[0]->buf; > + > + vaddr = mr->dmabuf_map->vaddr; dma_buf_map can be an __iomem too, you shouldn't dig around in this, but use the dma-buf-map.h helpers instead. On x86 (and I think also on most arm) it doesn't matter, but it's kinda not very nice in a pure software driver. If anything is missing in dma-buf-map.h wrappers just add more. Or alternatively you need to fail the import if you can't handle __iomem. Aside from these I think the dma-buf side here for cpu access looks reasonable now. -Daniel > + vaddr_end = vaddr + umem_dmabuf->dmabuf->size; > + remain = umem_dmabuf->dmabuf->size; > + > + for (; remain; vaddr += PAGE_SIZE) { > + if (num_buf >= RXE_BUF_PER_MAP) { > + map++; > + buf = map[0]->buf; > + num_buf = 0; > + } > + > + buf->addr = (uintptr_t)vaddr; > + if (remain >= PAGE_SIZE) > + buf->size = PAGE_SIZE; > + else > + buf->size = remain; > + remain -= buf->size; > + > + num_buf++; > + buf++; > + } > + > + return 0; > + > +err_free_dmabuf_map: > + kfree(mr->dmabuf_map); > +err_out: > + return err; > +} > + > +static void rxe_unmap_dmabuf_mr(struct rxe_mr *mr) > +{ > + struct ib_umem_dmabuf *umem_dmabuf = to_ib_umem_dmabuf(mr->umem); > + > + dma_buf_vunmap(umem_dmabuf->dmabuf, mr->dmabuf_map); > + kfree(mr->dmabuf_map); > +} > + > +int rxe_mr_dmabuf_init_user(struct rxe_pd *pd, int fd, u64 start, u64 length, > + u64 iova, int access, struct rxe_mr *mr) > +{ > + struct ib_umem_dmabuf *umem_dmabuf; > + struct rxe_map_set *set; > + int err; > + > + umem_dmabuf = ib_umem_dmabuf_get(pd->ibpd.device, start, length, fd, > + access, NULL); > + if (IS_ERR(umem_dmabuf)) { > + err = PTR_ERR(umem_dmabuf); > + goto err_out; > + } > + > + rxe_mr_init(access, mr); > + > + err = rxe_mr_alloc(mr, ib_umem_num_pages(&umem_dmabuf->umem), 0); > + if (err) { > + pr_warn("%s: Unable to allocate memory for map\n", __func__); > + goto err_release_umem; > + } > + > + mr->ibmr.pd = &pd->ibpd; > + mr->umem = &umem_dmabuf->umem; > + mr->access = access; > + mr->state = RXE_MR_STATE_VALID; > + mr->type = IB_MR_TYPE_USER; > + > + set = mr->cur_map_set; > + set->length = length; > + set->iova = iova; > + set->va = start; > + set->offset = ib_umem_offset(mr->umem); > + > + err = rxe_map_dmabuf_mr(mr, umem_dmabuf); > + if (err) > + goto err_free_map_set; > + > + return 0; > + > +err_free_map_set: > + rxe_mr_free_map_set(mr->num_map, mr->cur_map_set); > +err_release_umem: > + ib_umem_release(&umem_dmabuf->umem); > +err_out: > + return err; > +} > + > int rxe_mr_init_fast(struct rxe_pd *pd, int max_pages, struct rxe_mr *mr) > { > int err; > @@ -703,6 +818,9 @@ void rxe_mr_cleanup(struct rxe_pool_entry *arg) > { > struct rxe_mr *mr = container_of(arg, typeof(*mr), pelem); > > + if (mr->umem && mr->umem->is_dmabuf) > + rxe_unmap_dmabuf_mr(mr); > + > ib_umem_release(mr->umem); > > if (mr->cur_map_set) > diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c > index 9d0bb9aa7514..6191bb4f434d 100644 > --- a/drivers/infiniband/sw/rxe/rxe_verbs.c > +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c > @@ -916,6 +916,39 @@ static struct ib_mr *rxe_reg_user_mr(struct ib_pd *ibpd, > return ERR_PTR(err); > } > > +static struct ib_mr *rxe_reg_user_mr_dmabuf(struct ib_pd *ibpd, u64 start, > + u64 length, u64 iova, int fd, > + int access, struct ib_udata *udata) > +{ > + int err; > + struct rxe_dev *rxe = to_rdev(ibpd->device); > + struct rxe_pd *pd = to_rpd(ibpd); > + struct rxe_mr *mr; > + > + mr = rxe_alloc(&rxe->mr_pool); > + if (!mr) { > + err = -ENOMEM; > + goto err2; > + } > + > + rxe_add_index(mr); > + > + rxe_add_ref(pd); > + > + err = rxe_mr_dmabuf_init_user(pd, fd, start, length, iova, access, mr); > + if (err) > + goto err3; > + > + return &mr->ibmr; > + > +err3: > + rxe_drop_ref(pd); > + rxe_drop_index(mr); > + rxe_drop_ref(mr); > +err2: > + return ERR_PTR(err); > +} > + > static struct ib_mr *rxe_alloc_mr(struct ib_pd *ibpd, enum ib_mr_type mr_type, > u32 max_num_sg) > { > @@ -1081,6 +1114,7 @@ static const struct ib_device_ops rxe_dev_ops = { > .query_qp = rxe_query_qp, > .query_srq = rxe_query_srq, > .reg_user_mr = rxe_reg_user_mr, > + .reg_user_mr_dmabuf = rxe_reg_user_mr_dmabuf, > .req_notify_cq = rxe_req_notify_cq, > .resize_cq = rxe_resize_cq, > > diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h > index c807639435eb..0aa95ab06b6e 100644 > --- a/drivers/infiniband/sw/rxe/rxe_verbs.h > +++ b/drivers/infiniband/sw/rxe/rxe_verbs.h > @@ -334,6 +334,8 @@ struct rxe_mr { > > struct rxe_map_set *cur_map_set; > struct rxe_map_set *next_map_set; > + > + struct dma_buf_map *dmabuf_map; > }; > > enum rxe_mw_state { > -- > 2.17.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 6 months

Re: [Linaro-mm-sig] [PATCH v6 0/2] Add p2p via dmabuf to habanalabs

by Daniel Vetter

On Tue, Sep 28, 2021 at 10:04:29AM +0300, Oded Gabbay wrote: > On Thu, Sep 23, 2021 at 12:22 PM Oded Gabbay <ogabbay(a)kernel.org> wrote: > > > > On Sat, Sep 18, 2021 at 11:38 AM Oded Gabbay <ogabbay(a)kernel.org> wrote: > > > > > > On Fri, Sep 17, 2021 at 3:30 PM Daniel Vetter <daniel(a)ffwll.ch> wrote: > > > > > > > > On Thu, Sep 16, 2021 at 10:10:14AM -0300, Jason Gunthorpe wrote: > > > > > On Thu, Sep 16, 2021 at 02:31:34PM +0200, Daniel Vetter wrote: > > > > > > On Wed, Sep 15, 2021 at 10:45:36AM +0300, Oded Gabbay wrote: > > > > > > > On Tue, Sep 14, 2021 at 7:12 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > > > > > > > > > > > > > On Tue, Sep 14, 2021 at 04:18:31PM +0200, Daniel Vetter wrote: > > > > > > > > > On Sun, Sep 12, 2021 at 07:53:07PM +0300, Oded Gabbay wrote: > > > > > > > > > > Hi, > > > > > > > > > > Re-sending this patch-set following the release of our user-space TPC > > > > > > > > > > compiler and runtime library. > > > > > > > > > > > > > > > > > > > > I would appreciate a review on this. > > > > > > > > > > > > > > > > > > I think the big open we have is the entire revoke discussions. Having the > > > > > > > > > option to let dma-buf hang around which map to random local memory ranges, > > > > > > > > > without clear ownership link and a way to kill it sounds bad to me. > > > > > > > > > > > > > > > > > > I think there's a few options: > > > > > > > > > - We require revoke support. But I've heard rdma really doesn't like that, > > > > > > > > > I guess because taking out an MR while holding the dma_resv_lock would > > > > > > > > > be an inversion, so can't be done. Jason, can you recap what exactly the > > > > > > > > > hold-up was again that makes this a no-go? > > > > > > > > > > > > > > > > RDMA HW can't do revoke. > > > > > > > > > > > > Like why? I'm assuming when the final open handle or whatever for that MR > > > > > > is closed, you do clean up everything? Or does that MR still stick around > > > > > > forever too? > > > > > > > > > > It is a combination of uAPI and HW specification. > > > > > > > > > > revoke here means you take a MR object and tell it to stop doing DMA > > > > > without causing the MR object to be destructed. > > > > > > > > > > All the drivers can of course destruct the MR, but doing such a > > > > > destruction without explicit synchronization with user space opens > > > > > things up to a serious use-after potential that could be a security > > > > > issue. > > > > > > > > > > When the open handle closes the userspace is synchronized with the > > > > > kernel and we can destruct the HW objects safely. > > > > > > > > > > So, the special HW feature required is 'stop doing DMA but keep the > > > > > object in an error state' which isn't really implemented, and doesn't > > > > > extend very well to other object types beyond simple MRs. > > > > > > > > Yeah revoke without destroying the MR doesn't work, and it sounds like > > > > revoke by destroying the MR just moves the can of worms around to another > > > > place. > > > > > > > > > > 1. User A opens gaudi device, sets up dma-buf export > > > > > > > > > > > > 2. User A registers that with RDMA, or anything else that doesn't support > > > > > > revoke. > > > > > > > > > > > > 3. User A closes gaudi device > > > > > > > > > > > > 4. User B opens gaudi device, assumes that it has full control over the > > > > > > device and uploads some secrets, which happen to end up in the dma-buf > > > > > > region user A set up > > > > > > > > > > I would expect this is blocked so long as the DMABUF exists - eg the > > > > > DMABUF will hold a fget on the FD of #1 until the DMABUF is closed, so > > > > > that #3 can't actually happen. > > > > > > > > > > > It's not mlocked memory, it's mlocked memory and I can exfiltrate > > > > > > it. > > > > > > > > > > That's just bug, don't make buggy drivers :) > > > > > > > > Well yeah, but given that habanalabs hand rolled this I can't just check > > > > for the usual things we have to enforce this in drm. And generally you can > > > > just open chardevs arbitrarily, and multiple users fighting over each > > > > another. The troubles only start when you have private state or memory > > > > allocations of some kind attached to the struct file (instead of the > > > > underlying device), or something else that requires device exclusivity. > > > > There's no standard way to do that. > > > > > > > > Plus in many cases you really want revoke on top (can't get that here > > > > unfortunately it seems), and the attempts to get towards a generic > > > > revoke() just never went anywhere. So again it's all hand-rolled > > > > per-subsystem. *insert lament about us not having done this through a > > > > proper subsystem* > > > > > > > > Anyway it sounds like the code takes care of that. > > > > -Daniel > > > > > > Daniel, Jason, > > > Thanks for reviewing this code. > > > > > > Can I get an R-B / A-B from you for this patch-set ? > > > > > > Thanks, > > > Oded > > > > A kind reminder. > > > > Thanks, > > Oded > > Hi, > I know last week was LPC and maybe this got lost in the inbox, so I'm > sending it again to make sure you got my request for R-B / A-B. I was waiting for some clarity from the maintainers summit, but that's still about as unclear as it gets. Either way technically it sounds ok, but I'm a bit burried so didn't look at the code. Acked-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> But looking beyond the strict lens of dma-buf I'm still impressed by the mess this created, to get to the same endpoint of "we open our stack" in the same time it takes others to sort this out. I'm still looking for some kind of plan to fix this. Also you probably want to get Dave to ack this too, I pinged him on irc last week about this after maintainer summit. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 6 months

Re: [Linaro-mm-sig] [PATCH v6 2/2] habanalabs: add support for dma-buf exporter

by Jason Gunthorpe

On Sun, Sep 12, 2021 at 07:53:09PM +0300, Oded Gabbay wrote: > From: Tomer Tayar <ttayar(a)habana.ai> > > Implement the calls to the dma-buf kernel api to create a dma-buf > object backed by FD. > > We block the option to mmap the DMA-BUF object because we don't support > DIRECT_IO and implicit P2P. This statement doesn't make sense, you can mmap your dmabuf if you like. All dmabuf mmaps are supposed to set the special bit/etc to exclude them from get_user_pages() anyhow - and since this is BAR memory not struct page memory this driver would be doing it anyhow. > We check the p2p distance using pci_p2pdma_distance_many() and refusing > to map dmabuf in case the distance doesn't allow p2p. Does this actually allow the p2p transfer for your intended use cases? > diff --git a/drivers/misc/habanalabs/common/memory.c b/drivers/misc/habanalabs/common/memory.c > index 33986933aa9e..8cf5437c0390 100644 > +++ b/drivers/misc/habanalabs/common/memory.c > @@ -1,7 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0 > > /* > - * Copyright 2016-2019 HabanaLabs, Ltd. > + * Copyright 2016-2021 HabanaLabs, Ltd. > * All Rights Reserved. > */ > > @@ -11,11 +11,13 @@ > > #include <linux/uaccess.h> > #include <linux/slab.h> > +#include <linux/pci-p2pdma.h> > > #define HL_MMU_DEBUG 0 > > /* use small pages for supporting non-pow2 (32M/40M/48M) DRAM phys page sizes */ > -#define DRAM_POOL_PAGE_SIZE SZ_8M > +#define DRAM_POOL_PAGE_SIZE SZ_8M > + ?? > /* > * The va ranges in context object contain a list with the available chunks of > @@ -347,6 +349,13 @@ static int free_device_memory(struct hl_ctx *ctx, struct hl_mem_in *args) > return -EINVAL; > } > > + if (phys_pg_pack->exporting_cnt) { > + dev_err(hdev->dev, > + "handle %u is exported, cannot free\n", handle); > + spin_unlock(&vm->idr_lock); Don't write to the kernel log from user space triggered actions > +static int alloc_sgt_from_device_pages(struct hl_device *hdev, > + struct sg_table **sgt, u64 *pages, > + u64 npages, u64 page_size, > + struct device *dev, > + enum dma_data_direction dir) Why doesn't this return a sg_table * and an ERR_PTR? > +{ > + u64 chunk_size, bar_address, dma_max_seg_size; > + struct asic_fixed_properties *prop; > + int rc, i, j, nents, cur_page; > + struct scatterlist *sg; > + > + prop = &hdev->asic_prop; > + > + dma_max_seg_size = dma_get_max_seg_size(dev); > + > + /* We would like to align the max segment size to PAGE_SIZE, so the > + * SGL will contain aligned addresses that can be easily mapped to > + * an MMU > + */ > + dma_max_seg_size = ALIGN_DOWN(dma_max_seg_size, PAGE_SIZE); > + if (dma_max_seg_size < PAGE_SIZE) { > + dev_err_ratelimited(hdev->dev, > + "dma_max_seg_size %llu can't be smaller than PAGE_SIZE\n", > + dma_max_seg_size); > + return -EINVAL; > + } > + > + *sgt = kzalloc(sizeof(**sgt), GFP_KERNEL); > + if (!*sgt) > + return -ENOMEM; > + > + /* If the size of each page is larger than the dma max segment size, > + * then we can't combine pages and the number of entries in the SGL > + * will just be the > + * <number of pages> * <chunks of max segment size in each page> > + */ > + if (page_size > dma_max_seg_size) > + nents = npages * DIV_ROUND_UP_ULL(page_size, dma_max_seg_size); > + else > + /* Get number of non-contiguous chunks */ > + for (i = 1, nents = 1, chunk_size = page_size ; i < npages ; i++) { > + if (pages[i - 1] + page_size != pages[i] || > + chunk_size + page_size > dma_max_seg_size) { > + nents++; > + chunk_size = page_size; > + continue; > + } > + > + chunk_size += page_size; > + } > + > + rc = sg_alloc_table(*sgt, nents, GFP_KERNEL | __GFP_ZERO); > + if (rc) > + goto error_free; > + > + /* Because we are not going to include a CPU list we want to have some > + * chance that other users will detect this by setting the orig_nents > + * to 0 and using only nents (length of DMA list) when going over the > + * sgl > + */ > + (*sgt)->orig_nents = 0; Maybe do this at the end so you'd have to undo it on the error path? > + cur_page = 0; > + > + if (page_size > dma_max_seg_size) { > + u64 size_left, cur_device_address = 0; > + > + size_left = page_size; > + > + /* Need to split each page into the number of chunks of > + * dma_max_seg_size > + */ > + for_each_sgtable_dma_sg((*sgt), sg, i) { > + if (size_left == page_size) > + cur_device_address = > + pages[cur_page] - prop->dram_base_address; > + else > + cur_device_address += dma_max_seg_size; > + > + chunk_size = min(size_left, dma_max_seg_size); > + > + bar_address = hdev->dram_pci_bar_start + cur_device_address; > + > + rc = set_dma_sg(sg, bar_address, chunk_size, dev, dir); > + if (rc) > + goto error_unmap; > + > + if (size_left > dma_max_seg_size) { > + size_left -= dma_max_seg_size; > + } else { > + cur_page++; > + size_left = page_size; > + } > + } > + } else { > + /* Merge pages and put them into the scatterlist */ > + for_each_sgtable_dma_sg((*sgt), sg, i) { > + chunk_size = page_size; > + for (j = cur_page + 1 ; j < npages ; j++) { > + if (pages[j - 1] + page_size != pages[j] || > + chunk_size + page_size > dma_max_seg_size) > + break; > + > + chunk_size += page_size; > + } > + > + bar_address = hdev->dram_pci_bar_start + > + (pages[cur_page] - prop->dram_base_address); > + > + rc = set_dma_sg(sg, bar_address, chunk_size, dev, dir); > + if (rc) > + goto error_unmap; > + > + cur_page = j; > + } > + } We have this sg_append stuff now that is intended to help building these things. It can only build CPU page lists, not these DMA lists, but I do wonder if open coding in drivers is slipping back a bit. Especially since AMD seems to be doing something different. Could the DMABUF layer gain some helpers styled after the sg_append to simplify building these things? and convert the AMD driver of course. > +static int hl_dmabuf_attach(struct dma_buf *dmabuf, > + struct dma_buf_attachment *attachment) > +{ > + struct hl_dmabuf_wrapper *hl_dmabuf; > + struct hl_device *hdev; > + int rc; > + > + hl_dmabuf = dmabuf->priv; > + hdev = hl_dmabuf->ctx->hdev; > + > + rc = pci_p2pdma_distance_many(hdev->pdev, &attachment->dev, 1, true); > + > + if (rc < 0) > + attachment->peer2peer = false; Extra blank line > + > + return 0; > +} > + > +static struct sg_table *hl_map_dmabuf(struct dma_buf_attachment *attachment, > + enum dma_data_direction dir) > +{ > + struct dma_buf *dma_buf = attachment->dmabuf; > + struct hl_vm_phys_pg_pack *phys_pg_pack; > + struct hl_dmabuf_wrapper *hl_dmabuf; > + struct hl_device *hdev; > + struct sg_table *sgt; > + int rc; > + > + hl_dmabuf = dma_buf->priv; > + hdev = hl_dmabuf->ctx->hdev; > + phys_pg_pack = hl_dmabuf->phys_pg_pack; > + > + if (!attachment->peer2peer) { > + dev_err(hdev->dev, > + "Failed to map dmabuf because p2p is disabled\n"); > + return ERR_PTR(-EPERM); User triggered printable again? > +static void hl_unmap_dmabuf(struct dma_buf_attachment *attachment, > + struct sg_table *sgt, > + enum dma_data_direction dir) > +{ > + struct scatterlist *sg; > + int i; > + > + for_each_sgtable_dma_sg(sgt, sg, i) > + dma_unmap_resource(attachment->dev, sg_dma_address(sg), > + sg_dma_len(sg), dir, > + DMA_ATTR_SKIP_CPU_SYNC); Why can we skip the CPU_SYNC? Seems like a comment is needed Something has to do a CPU_SYNC before recylcing this memory for another purpose, where is it? > +static void hl_release_dmabuf(struct dma_buf *dmabuf) > +{ > + struct hl_dmabuf_wrapper *hl_dmabuf = dmabuf->priv; Maybe hl_dmabuf_wrapper should be hl_dmabuf_priv > + * export_dmabuf_from_addr() - export a dma-buf object for the given memory > + * address and size. > + * @ctx: pointer to the context structure. > + * @device_addr: device memory physical address. > + * @size: size of device memory. > + * @flags: DMA-BUF file/FD flags. > + * @dmabuf_fd: pointer to result FD that represents the dma-buf object. > + * > + * Create and export a dma-buf object for an existing memory allocation inside > + * the device memory, and return a FD which is associated with the dma-buf > + * object. > + * > + * Return: 0 on success, non-zero for failure. > + */ > +static int export_dmabuf_from_addr(struct hl_ctx *ctx, u64 device_addr, > + u64 size, int flags, int *dmabuf_fd) > +{ > + struct hl_dmabuf_wrapper *hl_dmabuf; > + struct hl_device *hdev = ctx->hdev; > + struct asic_fixed_properties *prop; > + u64 bar_address; > + int rc; > + > + prop = &hdev->asic_prop; > + > + if (!IS_ALIGNED(device_addr, PAGE_SIZE)) { > + dev_err_ratelimited(hdev->dev, > + "address of exported device memory should be aligned to 0x%lx, address 0x%llx\n", > + PAGE_SIZE, device_addr); > + return -EINVAL; > + } > + > + if (size < PAGE_SIZE) { > + dev_err_ratelimited(hdev->dev, > + "size %llu of exported device memory should be equal to or greater than %lu\n", > + size, PAGE_SIZE); > + return -EINVAL; > + } > + > + if (device_addr < prop->dram_user_base_address || > + device_addr + size > prop->dram_end_address || > + device_addr + size < device_addr) { > + dev_err_ratelimited(hdev->dev, > + "DRAM memory range is outside of DRAM boundaries, address 0x%llx, size 0x%llx\n", > + device_addr, size); > + return -EINVAL; > + } > + > + bar_address = hdev->dram_pci_bar_start + > + (device_addr - prop->dram_base_address); > + > + if (bar_address + size > > + hdev->dram_pci_bar_start + prop->dram_pci_bar_size || > + bar_address + size < bar_address) { > + dev_err_ratelimited(hdev->dev, > + "DRAM memory range is outside of PCI BAR boundaries, address 0x%llx, size 0x%llx\n", > + device_addr, size); > + return -EINVAL; > + } More prints from userspace > +static int export_dmabuf_from_handle(struct hl_ctx *ctx, u64 handle, int flags, > + int *dmabuf_fd) > +{ > + struct hl_vm_phys_pg_pack *phys_pg_pack; > + struct hl_dmabuf_wrapper *hl_dmabuf; > + struct hl_device *hdev = ctx->hdev; > + struct asic_fixed_properties *prop; > + struct hl_vm *vm = &hdev->vm; > + u64 bar_address; > + u32 idr_handle; > + int rc, i; > + > + prop = &hdev->asic_prop; > + > + idr_handle = lower_32_bits(handle); Why silent truncation? Shouldn't setting the upper 32 bits be an error? > + case HL_MEM_OP_EXPORT_DMABUF_FD: > + rc = export_dmabuf_from_addr(ctx, > + args->in.export_dmabuf_fd.handle, > + args->in.export_dmabuf_fd.mem_size, > + args->in.flags, > + &dmabuf_fd); > + memset(args, 0, sizeof(*args)); > + args->out.fd = dmabuf_fd; Would expect the installed fd to be the positive return, not a pointer Jason

3 years, 6 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig