Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

5 participants
2538 discussions

Re: [PATCH] drm/nouveau: Prevents NULL pointer dereference in nouveau_uvmm_sm_prepare

by Danilo Krummrich

Hi Yuran, On 10/26/23 19:03, Yuran Pereira wrote: > There are instances where the "args" argument passed to > nouveau_uvmm_sm_prepare() is NULL. > > I.e. when nouveau_uvmm_sm_prepare() is called from > nouveau_uvmm_sm_unmap_prepare() > > ``` > static int > nouveau_uvmm_sm_unmap_prepare(struct nouveau_uvmm *uvmm, > ... > { > return nouveau_uvmm_sm_prepare(uvmm, new, ops, NULL); > } > ``` > > The problem is that op_map_prepare() which nouveau_uvmm_sm_prepare > calls, dereferences this value, which can lead to a NULL pointer > dereference. op_map_prepare() can't be called with `args` being NULL, since when called through nouveau_uvmm_sm_unmap_prepare() we can't hit the DRM_GPUVA_OP_MAP case at all. Unmapping something never leads to a new mapping being created, it can lead to remaps though. > > ``` > static int > op_map_prepare(struct nouveau_uvmm *uvmm, > ... > { > ... > uvma->region = args->region; <-- Dereferencing of possibly NULL pointer > uvma->kind = args->kind; <-- > ... > } > ``` > > ``` > static int > nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm, > ... > struct uvmm_map_args *args) > { > struct drm_gpuva_op *op; > u64 vmm_get_start = args ? args->addr : 0; > u64 vmm_get_end = args ? args->addr + args->range : 0; > int ret; > > drm_gpuva_for_each_op(op, ops) { > switch (op->op) { > case DRM_GPUVA_OP_MAP: { > u64 vmm_get_range = vmm_get_end - vmm_get_start; > > ret = op_map_prepare(uvmm, &new->map, &op->map, args); <--- > if (ret) > goto unwind; > > if (args && vmm_get_range) { > ret = nouveau_uvmm_vmm_get(uvmm, vmm_get_start, > vmm_get_range); > if (ret) { > op_map_prepare_unwind(new->map); > goto unwind; > } > } > ... > ``` > > Since the switch "case DRM_GPUVA_OP_MAP", also NULL checks "args" This check is not required for the reason given above. If you like, you can change this patch up to remove the args check and add a comment like: /* args can't be NULL when called for a map operation. */ > after the call to op_map_prepare(), my guess is that we should > probably relocate this check to a point before op_map_prepare() > is called. Yeah, I see how this unnecessary check made you think so. - Danilo > > This patch ensures that the value of args is checked before > calling op_map_prepare() > > Addresses-Coverity-ID: 1544574 ("Dereference after null check") > Signed-off-by: Yuran Pereira <yuran.pereira(a)hotmail.com> > --- > drivers/gpu/drm/nouveau/nouveau_uvmm.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c > index aae780e4a4aa..6baa481eb2c8 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c > @@ -620,11 +620,14 @@ nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm, > case DRM_GPUVA_OP_MAP: { > u64 vmm_get_range = vmm_get_end - vmm_get_start; > > + if (!args) > + goto unwind; > + > ret = op_map_prepare(uvmm, &new->map, &op->map, args); > if (ret) > goto unwind; > > - if (args && vmm_get_range) { > + if (vmm_get_range) { > ret = nouveau_uvmm_vmm_get(uvmm, vmm_get_start, > vmm_get_range); > if (ret) {

1 year, 1 month

Re: [RFC PATCH v3 12/12] selftests: add ncdevmem, netcat for devmem TCP

by Jakub Kicinski

On Sun, 12 Nov 2023 20:08:10 -0800 Mina Almasry wrote: > 1. For (b), would it be OK to implement a very minimal version of > queue_[stop|start]/queue_mem_[alloc|free], which I use for the sole > purpose of reposting buffers to an individual queue, and then later > whoever picks up your queue API effort (maybe me) extends the > implementation to do the rest of the things you described in your > email? If not, what is the minimal queue API I can implement and use > for devmem TCP? Any form of queue API is better than a temporary ndo. IIUC it will not bubble up into uAPI in any way so we can extend/change it later as needed. > 2. Since this is adding ndo, do I need to implement the ndo for 2 > drivers or is GVE sufficient? One driver is fine, especially if we're doing this instead of the reset hack.

1 year, 1 month

Re: [RFC PATCH v3 08/12] net: support non paged skb frags

by Jakub Kicinski

On Sun, 12 Nov 2023 22:05:30 -0800 Mina Almasry wrote: > My issue here is that all these skb helpers call each other so I end > up having to move a lot of the unrelated skb helpers to this new > header (maybe that is acceptable but it feels weird). Splitting pp headers again is not an option, we already split it into types and helpers. The series doesn't apply and it's a bit hard for me to, in between LPC talks, figure out how to extract your patches from the GH UI to take a proper look :( We can defer this for now, and hopefully v4 will apply to net-next. But it will need to get solved.

1 year, 1 month

Re: [RFC PATCH v3 02/12] net: page_pool: create hooks for custom page providers

by Jakub Kicinski

On Sun, 12 Nov 2023 19:28:52 -0800 Mina Almasry wrote: > My issue with this is that if the driver doesn't support dmabuf then > the driver will accidentally use the pp backed by the dmabuf, allocate > a page from it, then call page_address() on it or something, and > crash. > > Currently I avoid that by having the driver be responsible for picking > up the dmabuf from the netdev_rx_queue and giving it to the page pool. > What would be the appropriate way to check for driver support in the > netlink API? Perhaps adding something to ndo_features_check? We need some form of capabilities. I was expecting to add that as part of the queue API. Either a new field in struct net_device or in ndos. I tend to put static driver caps of this nature into ops. See for instance .supported_ring_params in ethtool ops.

1 year, 1 month

Re: [RFC PATCH v3 12/12] selftests: add ncdevmem, netcat for devmem TCP

by Jakub Kicinski

On Fri, 10 Nov 2023 18:27:08 -0800 Mina Almasry wrote: > Thanks for the clear requirement. I clearly had something different in mind. > > Might be dumb suggestions, but instead of creating a new ndo that we > maybe end up wanting to deprecate once the queue API is ready, how > about we use either of those existing APIs? > > +void netdev_reset(struct net_device *dev) > +{ > + int flags = ETH_RESET_ALL; > + int err; > + > +#if 1 > + __dev_close(dev); > + err = __dev_open(dev, NULL); > +#else > + err = dev->ethtool_ops->reset(dev, &flags); > +#endif > +} > + > > I've tested both of these to work with GVE on both bind via the > netlink API and unbind via the netlink socket close, but I'm not > enough of an expert to tell if there is some bad side effect that can > happen or something. We generally don't accept drivers doing device reconfiguration with full close() + open() because if the open() fails your machine may be cut off. There are drivers which do it, but they are either old... or weren't reviewed hard enough. The driver should allocate memory and whether else it can without stopping the queues first. Once it has all those, stop the queues, reconfigure with already allocated resources, start queues, free old. Even without the queue API in place, good drivers do full device reconfig this way. Hence my mind goes towards a new (temporary?) ndo. It will be replaced by the queue API, but whoever implements it for now has to follow this careful reconfig strategy...

1 year, 2 months

Re: [RFC PATCH v3 04/12] netdev: support binding dma-buf to netdevice

by Jakub Kicinski

On Sun, 5 Nov 2023 18:44:03 -0800 Mina Almasry wrote: > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -52,6 +52,8 @@ > #include <net/net_trackers.h> > #include <net/net_debug.h> > #include <net/dropreason-core.h> > +#include <linux/xarray.h> > +#include <linux/refcount.h> > > struct netpoll_info; > struct device; > @@ -808,6 +810,84 @@ bool rps_may_expire_flow(struct net_device *dev, u16 rxq_index, u32 flow_id, > #endif > #endif /* CONFIG_RPS */ > > +struct netdev_dmabuf_binding { Similar nitpick to the skbuff.h comment. Take this somewhere else, please, it doesn't need to be included in netdevice.h > + struct netdev_dmabuf_binding *rbinding; the 'r' in rbinding stands for rx? 🤔️

1 year, 2 months

Re: [RFC PATCH v3 08/12] net: support non paged skb frags

by Jakub Kicinski

On Sun, 5 Nov 2023 18:44:07 -0800 Mina Almasry wrote: > #include <net/net_debug.h> > #include <net/dropreason-core.h> > +#include <net/page_pool/types.h> > +#include <net/page_pool/helpers.h> > /** > * DOC: skb checksums > @@ -3402,15 +3404,38 @@ static inline void skb_frag_off_copy(skb_frag_t *fragto, > fragto->bv_offset = fragfrom->bv_offset; > } > > +/* Returns true if the skb_frag contains a page_pool_iov. */ > +static inline bool skb_frag_is_page_pool_iov(const skb_frag_t *frag) > +{ > + return page_is_page_pool_iov(frag->bv_page); > +} Maybe we can create a new header? For skb + page pool. skbuff.h is included by 1/4th of the kernel objects, we should not be adding dependencies to this header, it really slows down incremental builds.

1 year, 2 months

Re: [RFC PATCH v3 09/12] net: add support for skbs with unreadable frags

by Jakub Kicinski

On Tue, 7 Nov 2023 11:53:22 -0800 Mina Almasry wrote: > My bad on not including some docs about this. The next version should > have the commit message beefed up to explain all this, or a docs > patch. Yes, please. Would be great to have the user facing interface well explained under Documentation/

1 year, 2 months

Re: [RFC PATCH v3 10/12] tcp: RX path for devmem TCP

by Jakub Kicinski

On Sun, 5 Nov 2023 18:44:09 -0800 Mina Almasry wrote: > + if (!skb_frags_not_readable(skb)) { nit: maybe call the helper skb_frags_readable() and flip the polarity, avoid double negatives.

1 year, 2 months

Re: [RFC PATCH v3 02/12] net: page_pool: create hooks for custom page providers

by Jakub Kicinski

On Sun, 5 Nov 2023 18:44:01 -0800 Mina Almasry wrote: > diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h > index 6fc5134095ed..d4bea053bb7e 100644 > --- a/include/net/page_pool/types.h > +++ b/include/net/page_pool/types.h > @@ -60,6 +60,8 @@ struct page_pool_params { > int nid; > struct device *dev; > struct napi_struct *napi; > + u8 memory_provider; > + void *mp_priv; > enum dma_data_direction dma_dir; > unsigned int max_len; > unsigned int offset; you should rebase on top of net-next More importantly I was expecting those fields to be gone from params. The fact that the page pool is configured to a specific provider should be fully transparent to the driver, driver should just tell the core what queue its creating the pool from and if there's a dmabuf bound for that queue - out pops a pp backed by the dmabuf. My RFC had the page pool params fields here as a hack.

1 year, 2 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig