2018-07-23 18:41 GMT+09:00 Björn Töpel <bjorn.to...@gmail.com>:
> Den lör 21 juli 2018 kl 14:58 skrev Taehee Yoo <ap420...@gmail.com>:
>>
>> 2018-07-21 2:18 GMT+09:00 Martin KaFai Lau <ka...@fb.com>:
>> > On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote:
>> >> rhashtable_lookup() can return NULL. so that NULL pointer
>> >> check routine should be added.
>> >>
>> >> Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
>> >> Signed-off-by: Taehee Yoo <ap420...@gmail.com>
>> >> ---
>> >> net/core/xdp.c | 3 ++-
>> >> 1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> >> index 9d1f220..1c12bc7 100644
>> >> --- a/net/core/xdp.c
>> >> +++ b/net/core/xdp.c
>> >> @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct
>> >> xdp_mem_info *mem, bool napi_direct,
>> >> rcu_read_lock();
>> >> /* mem->id is valid, checked in
>> >> xdp_rxq_info_reg_mem_model() */
>> >> xa = rhashtable_lookup(mem_id_ht, &mem->id,
>> >> mem_id_rht_params);
>> >> - xa->zc_alloc->free(xa->zc_alloc, handle);
>> >> + if (xa)
>> >> + xa->zc_alloc->free(xa->zc_alloc, handle);
>> > hmm...It is not clear to me the "!xa" case don't have to be handled?
>>
>> Thank you for reviewing!
>>
>> Returning NULL pointer is bug case such as calling after use
>> xdp_rxq_info_unreg().
>> so that, I think it can't handle at that moment.
>> we can make __xdp_return to add WARN_ON_ONCE() or
>> add return error code to driver.
>> But I'm not sure if these is useful information.
>>
>> I might have misunderstood scenario of MEM_TYPE_ZERO_COPY
>> because there is no use case of MEM_TYPE_ZERO_COPY yet.
>>
>
> Taehee, again, sorry for the slow response and thanks for patch!
>
> If xa is NULL, the driver has a buggy/broken implementation. What
> would be a proper way of dealing with this? BUG?
>
Thank you for reviewing!
I would like to add WARN_ON_ONCE. because code writers can
get opportunity for debugging this in runtime. also I think this bug
doesn't make critical side effect.
Thanks!
>
> Björn
>
>> Thanks!
>>
>> >
>> >> rcu_read_unlock();
>> >> default:
>> >> /* Not possible, checked in xdp_rxq_info_reg_mem_model() */
>> >> --
>> >> 2.9.3
>> >>
