netdev - Re: [Intel-wired-lan] [PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <672730fc-2224-d5fe-87d0-7dc9b00bf207@huawei.com>
Date: Thu, 17 Oct 2024 11:55:05 +0800
From: Yue Haibing <yuehaibing@...wei.com>
To: Jacob Keller <jacob.e.keller@...el.com>, Simon Horman <horms@...nel.org>
CC: <anthony.l.nguyen@...el.com>, <przemyslaw.kitszel@...el.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <ast@...nel.org>, <daniel@...earbox.net>,
	<hawk@...nel.org>, <john.fastabend@...il.com>, <vedang.patel@...el.com>,
	<andre.guedes@...el.com>, <maciej.fijalkowski@...el.com>,
	<jithu.joseph@...el.com>, <intel-wired-lan@...ts.osuosl.org>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<bpf@...r.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCH net] igc: Fix passing 0 to ERR_PTR in
 igc_xdp_run_prog()

On 2024/10/17 7:12, Jacob Keller wrote:
> 
> 
> On 10/16/2024 4:06 PM, Jacob Keller wrote:
>>
>>
>> On 10/16/2024 11:53 AM, Simon Horman wrote:
>>> On Wed, Oct 16, 2024 at 06:53:10PM +0800, Yue Haibing wrote:
>>>> Return NULL instead of passing to ERR_PTR while res is IGC_XDP_PASS,
>>>> which is zero, this fix smatch warnings:
>>>> drivers/net/ethernet/intel/igc/igc_main.c:2533
>>>>  igc_xdp_run_prog() warn: passing zero to 'ERR_PTR'
>>>>
>>>> Fixes: 26575105d6ed ("igc: Add initial XDP support")
>>>> Signed-off-by: Yue Haibing <yuehaibing@...wei.com>
>>>> ---
>>>>  drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>>>> index 6e70bca15db1..c3d6e20c0be0 100644
>>>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>>>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>>>> @@ -2530,7 +2530,7 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter,
>>>>  	res = __igc_xdp_run_prog(adapter, prog, xdp);
>>>>  
>>>>  out:
>>>> -	return ERR_PTR(-res);
>>>> +	return res ? ERR_PTR(-res) : NULL;
>>>
>>> I think this is what PTR_ERR_OR_ZERO() is for.
>>
>> Not quite. PTR_ERR_OR_ZERO is intended for the case where you are
>> extracting an error from a pointer. This is converting an error into a
>> pointer.
>>
>> I am not sure what is really expected here. If res is zero, shouldn't we
>> be returning an skb pointer and not NULL?
>>
>> Why does igc_xdp_run_prog even return a sk_buff pointer at all? It never
>> actually returns an skb...
>>
>> This feels like the wrong fix entirely.
>>
>> __igc_xdp_run_prog returns a custom value for the action, between
>> IGC_XDP_PASS, IGC_XDP_TX, IGC_XDP_REDIRECT, or IGC_XDP_CONSUMED.
>>
>> This function is called by igc_xdp_run_prog which converts this to a
>> negative error code with the sk_buff pointer type.
>>
>> All so that we can assign a value to the skb pointer in
>> ice_clean_rx_irq, and check it with IS_ERR
>>
>> I don't like this fix, I think we could drop the igc_xdp_run_prog
>> wrapper, call __igc_xdp_run_prog directly and check its return value
>> instead of this method of using an error pointer.
> 
> Indeed, this SKB error stuff was added by 26575105d6ed ("igc: Add
> initial XDP support") which claims to be aligning with other Intel drivers.
> 

Thanks for review，maybe can fix this as commit 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c")?

> But the other Intel drivers just have a function that returns the xdp
> result and checks it directly.
> 
> Perhaps this is due to the way that the igc driver shares rings between
> XDP and the regular path?
> 
> Its not clear to me, but I think this fix is not what I would do.
> 
> .