| Message ID | YiC0BwndXiwxGDNz@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] xdp: xdp_mem_allocator can be NULL in trace_mem_connect(). | expand |
Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: > Since the commit mentioned below __xdp_reg_mem_model() can return a NULL > pointer. This pointer is dereferenced in trace_mem_connect() which leads > to segfault. It can be reproduced with enabled trace events during ifup. > > Only assign the arguments in the trace-event macro if `xa' is set. > Otherwise set the parameters to 0. > > Fixes: 4a48ef70b93b8 ("xdp: Allow registering memory model without rxq reference") > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Hmm, so before the commit you mention, the tracepoint wasn't triggered at all in the code path that now sets xdp_alloc is NULL. So I'm wondering if we should just do the same here? Is the trace event useful in all cases? Alternatively, if we keep it, I think the mem.id and mem.type should be available from rxq->mem, right? -Toke
On 2022-03-03 14:59:47 [+0100], Toke Høiland-Jørgensen wrote: > Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: > > > Since the commit mentioned below __xdp_reg_mem_model() can return a NULL > > pointer. This pointer is dereferenced in trace_mem_connect() which leads > > to segfault. It can be reproduced with enabled trace events during ifup. > > > > Only assign the arguments in the trace-event macro if `xa' is set. > > Otherwise set the parameters to 0. > > > > Fixes: 4a48ef70b93b8 ("xdp: Allow registering memory model without rxq reference") > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > > Hmm, so before the commit you mention, the tracepoint wasn't triggered > at all in the code path that now sets xdp_alloc is NULL. So I'm > wondering if we should just do the same here? Is the trace event useful > in all cases? Correct. It says: | ip-1230 [003] ..... 3.053473: mem_connect: mem_id=0 mem_type=PAGE_SHARED allocator=0000000000000000 ifindex=2 > Alternatively, if we keep it, I think the mem.id and mem.type should be > available from rxq->mem, right? Yes, if these are the same things. In my case they are also 0: | ip-1245 [000] ..... 3.045684: mem_connect: mem_id=0 mem_type=PAGE_SHARED allocator=0000000000000000 ifindex=2 | ifconfig-1332 [003] ..... 21.030879: mem_connect: mem_id=0 mem_type=PAGE_SHARED allocator=0000000000000000 ifindex=3 So depends on what makes sense that tp can be skipped for xa == NULL or remain with __entry->mem_id = rxq->mem.id; __entry->mem_type = rxq->mem.type; __entry->allocator = xa ? xa->allocator : NULL; if it makes sense. > -Toke Sebastian
On 03/03/2022 15.12, Sebastian Andrzej Siewior wrote: > On 2022-03-03 14:59:47 [+0100], Toke Høiland-Jørgensen wrote: >> Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: >> >>> Since the commit mentioned below __xdp_reg_mem_model() can return a NULL >>> pointer. This pointer is dereferenced in trace_mem_connect() which leads >>> to segfault. It can be reproduced with enabled trace events during ifup. >>> >>> Only assign the arguments in the trace-event macro if `xa' is set. >>> Otherwise set the parameters to 0. >>> >>> Fixes: 4a48ef70b93b8 ("xdp: Allow registering memory model without rxq reference") >>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> >> >> Hmm, so before the commit you mention, the tracepoint wasn't triggered >> at all in the code path that now sets xdp_alloc is NULL. So I'm >> wondering if we should just do the same here? Is the trace event useful >> in all cases? > > Correct. It says: > | ip-1230 [003] ..... 3.053473: mem_connect: mem_id=0 mem_type=PAGE_SHARED allocator=0000000000000000 ifindex=2 > >> Alternatively, if we keep it, I think the mem.id and mem.type should be >> available from rxq->mem, right? > > Yes, if these are the same things. In my case they are also 0: > > | ip-1245 [000] ..... 3.045684: mem_connect: mem_id=0 mem_type=PAGE_SHARED allocator=0000000000000000 ifindex=2 > | ifconfig-1332 [003] ..... 21.030879: mem_connect: mem_id=0 mem_type=PAGE_SHARED allocator=0000000000000000 ifindex=3 > > So depends on what makes sense that tp can be skipped for xa == NULL or > remain with > __entry->mem_id = rxq->mem.id; > __entry->mem_type = rxq->mem.type; > __entry->allocator = xa ? xa->allocator : NULL; > > if it makes sense. > I have two bpftrace scripts [1] and [2] that use this tracepoint. It is scripts to help driver developers detect memory leaks when using page_pool from their drivers. I'm a little pressured on time, so I've not evaluated if your change makes sense. In the scripts I do use both mem.id and mem.type. [1] https://github.com/xdp-project/xdp-project/blob/master/areas/mem/bpftrace/xdp_mem_track01.bt [2] https://github.com/xdp-project/xdp-project/blob/master/areas/mem/bpftrace/xdp_mem_track02.bt --Jesper
Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: > On 2022-03-03 14:59:47 [+0100], Toke Høiland-Jørgensen wrote: >> Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: >> >> > Since the commit mentioned below __xdp_reg_mem_model() can return a NULL >> > pointer. This pointer is dereferenced in trace_mem_connect() which leads >> > to segfault. It can be reproduced with enabled trace events during ifup. >> > >> > Only assign the arguments in the trace-event macro if `xa' is set. >> > Otherwise set the parameters to 0. >> > >> > Fixes: 4a48ef70b93b8 ("xdp: Allow registering memory model without rxq reference") >> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> >> >> Hmm, so before the commit you mention, the tracepoint wasn't triggered >> at all in the code path that now sets xdp_alloc is NULL. So I'm >> wondering if we should just do the same here? Is the trace event useful >> in all cases? > > Correct. It says: > | ip-1230 [003] ..... 3.053473: mem_connect: mem_id=0 mem_type=PAGE_SHARED allocator=0000000000000000 ifindex=2 > >> Alternatively, if we keep it, I think the mem.id and mem.type should be >> available from rxq->mem, right? > > Yes, if these are the same things. In my case they are also 0: > > | ip-1245 [000] ..... 3.045684: mem_connect: mem_id=0 mem_type=PAGE_SHARED allocator=0000000000000000 ifindex=2 > | ifconfig-1332 [003] ..... 21.030879: mem_connect: mem_id=0 mem_type=PAGE_SHARED allocator=0000000000000000 ifindex=3 > > So depends on what makes sense that tp can be skipped for xa == NULL or > remain with > __entry->mem_id = rxq->mem.id; > __entry->mem_type = rxq->mem.type; > __entry->allocator = xa ? xa->allocator : NULL; > > if it makes sense. Right, looking at the code again, the id is only assigned in the path that doesn't return NULL from __xdp_reg_mem_model(). Given that the trace points were put in specifically to be able to pair connect/disconnect using the IDs, I don't think there's any use to creating the events if there's no ID, so I think we should fix it by skipping the trace event entirely if xdp_alloc is NULL. -Toke
On 2022-03-07 17:50:04 [+0100], Toke Høiland-Jørgensen wrote: > > Right, looking at the code again, the id is only assigned in the path > that doesn't return NULL from __xdp_reg_mem_model(). > > Given that the trace points were put in specifically to be able to pair > connect/disconnect using the IDs, I don't think there's any use to > creating the events if there's no ID, so I think we should fix it by > skipping the trace event entirely if xdp_alloc is NULL. This sounds like a reasonable explanation. If nobody disagrees then I post a new patch tomorrow and try to recycle some of what you wrote :) > -Toke Sebastian
Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: > On 2022-03-07 17:50:04 [+0100], Toke Høiland-Jørgensen wrote: >> >> Right, looking at the code again, the id is only assigned in the path >> that doesn't return NULL from __xdp_reg_mem_model(). >> >> Given that the trace points were put in specifically to be able to pair >> connect/disconnect using the IDs, I don't think there's any use to >> creating the events if there's no ID, so I think we should fix it by >> skipping the trace event entirely if xdp_alloc is NULL. > > This sounds like a reasonable explanation. If nobody disagrees then I > post a new patch tomorrow and try to recycle some of what you wrote :) SGTM :) -Toke
On Mon, 07 Mar 2022 19:07:37 +0100 Toke Høiland-Jørgensen wrote: > Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: > > > On 2022-03-07 17:50:04 [+0100], Toke Høiland-Jørgensen wrote: > >> > >> Right, looking at the code again, the id is only assigned in the path > >> that doesn't return NULL from __xdp_reg_mem_model(). > >> > >> Given that the trace points were put in specifically to be able to pair > >> connect/disconnect using the IDs, I don't think there's any use to > >> creating the events if there's no ID, so I think we should fix it by > >> skipping the trace event entirely if xdp_alloc is NULL. > > > > This sounds like a reasonable explanation. If nobody disagrees then I > > post a new patch tomorrow and try to recycle some of what you wrote :) > > SGTM :) Was the patch posted? This seems to be a 5.17 thing, so it'd be really really good if the fix was in net by tomorrow morning! :S
On 2022-03-09 09:15:08 [-0800], Jakub Kicinski wrote: > Was the patch posted? This seems to be a 5.17 thing, so it'd be really > really good if the fix was in net by tomorrow morning! :S Just posted v2. Sebastian
On Wed, 9 Mar 2022 21:48:08 +0100 Sebastian Andrzej Siewior wrote: > On 2022-03-09 09:15:08 [-0800], Jakub Kicinski wrote: > > Was the patch posted? This seems to be a 5.17 thing, so it'd be really > > really good if the fix was in net by tomorrow morning! :S > > Just posted v2. Perfect, thank you!
diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h index c40fc97f94171..9196dcd08e6c7 100644 --- a/include/trace/events/xdp.h +++ b/include/trace/events/xdp.h @@ -362,9 +362,9 @@ TRACE_EVENT(mem_connect, TP_fast_assign( __entry->xa = xa; - __entry->mem_id = xa->mem.id; - __entry->mem_type = xa->mem.type; - __entry->allocator = xa->allocator; + __entry->mem_id = xa ? xa->mem.id : 0; + __entry->mem_type = xa ? xa->mem.type : 0; + __entry->allocator = xa ? xa->allocator : NULL; __entry->rxq = rxq; __entry->ifindex = rxq->dev->ifindex; ),
Since the commit mentioned below __xdp_reg_mem_model() can return a NULL pointer. This pointer is dereferenced in trace_mem_connect() which leads to segfault. It can be reproduced with enabled trace events during ifup. Only assign the arguments in the trace-event macro if `xa' is set. Otherwise set the parameters to 0. Fixes: 4a48ef70b93b8 ("xdp: Allow registering memory model without rxq reference") Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- include/trace/events/xdp.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)