Re: [Intel-wired-lan] [PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog()

[Jacob Keller <jacob.e.keller@intel.com>]

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@huawei.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.

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.