[PATCH net] xdp: xdp_mem_allocator can be NULL in trace_mem_connect().

[PATCH net] xdp: xdp_mem_allocator can be NULL in trace_mem_connect().

[Sebastian Andrzej Siewior]

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(-)

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;
 	),
-- 
2.35.1

Re: [PATCH net] xdp: xdp_mem_allocator can be NULL in trace_mem_connect().

[Toke Høiland-Jørgensen]

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

Re: [PATCH net] xdp: xdp_mem_allocator can be NULL in trace_mem_connect().

[Sebastian Andrzej Siewior]

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

Re: [PATCH net] xdp: xdp_mem_allocator can be NULL in trace_mem_connect().

[Jesper Dangaard Brouer]

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

Re: [PATCH net] xdp: xdp_mem_allocator can be NULL in trace_mem_connect().

[Toke Høiland-Jørgensen]

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

Re: [PATCH net] xdp: xdp_mem_allocator can be NULL in trace_mem_connect().

[Sebastian Andrzej Siewior]

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

Re: [PATCH net] xdp: xdp_mem_allocator can be NULL in trace_mem_connect().

[Toke Høiland-Jørgensen]

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

Re: [PATCH net] xdp: xdp_mem_allocator can be NULL in trace_mem_connect().

[Jakub Kicinski]

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

Re: [PATCH net] xdp: xdp_mem_allocator can be NULL in trace_mem_connect().

[Sebastian Andrzej Siewior]

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

Re: [PATCH net] xdp: xdp_mem_allocator can be NULL in trace_mem_connect().

[Jakub Kicinski]

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!