[Intel-wired-lan] [bug report] ixgbe: add VF IPsec management

[Intel-wired-lan] [bug report] ixgbe: add VF IPsec management

[Dan Carpenter]

Hello Shannon Nelson,

The patch eda0333ac293: "ixgbe: add VF IPsec management" from Aug 13,
2018 (linux-next), leads to the following Smatch static checker
warning:

	drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c:917 ixgbe_ipsec_vf_add_sa()
	warn: sleeping in IRQ context

drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
    890 int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
    891 {
    892         struct ixgbe_ipsec *ipsec = adapter->ipsec;
    893         struct xfrm_algo_desc *algo;
    894         struct sa_mbx_msg *sam;
    895         struct xfrm_state *xs;
    896         size_t aead_len;
    897         u16 sa_idx;
    898         u32 pfsa;
    899         int err;
    900 
    901         sam = (struct sa_mbx_msg *)(&msgbuf[1]);
    902         if (!adapter->vfinfo[vf].trusted ||
    903             !(adapter->flags2 & IXGBE_FLAG2_VF_IPSEC_ENABLED)) {
    904                 e_warn(drv, "VF %d attempted to add an IPsec SA\n", vf);
    905                 err = -EACCES;
    906                 goto err_out;
    907         }
    908 
    909         /* Tx IPsec offload doesn't seem to work on this
    910          * device, so block these requests for now.
    911          */
    912         if (sam->dir != XFRM_DEV_OFFLOAD_IN) {
    913                 err = -EOPNOTSUPP;
    914                 goto err_out;
    915         }
    916 
--> 917         xs = kzalloc(sizeof(*xs), GFP_KERNEL);
                                          ^^^^^^^^^^
Sleeping allocation.

The call tree that Smatch is worried about is:

ixgbe_msix_other() <- IRQ handler
-> ixgbe_msg_task()
   -> ixgbe_rcv_msg_from_vf()
      -> ixgbe_ipsec_vf_add_sa()

This is a fairly new warning and those have a higher risk of false
positives.  Plus the longer the call tree the higher the chance of
false positives.  However, I did review it and the warning looks
reasonable.

regards,
dan carpenter

Re: [Intel-wired-lan] [bug report] ixgbe: add VF IPsec management

[Nelson, Shannon]

On 2/9/2024 4:59 AM, Dan Carpenter wrote:
> 
> Hello Shannon Nelson,
> 
> The patch eda0333ac293: "ixgbe: add VF IPsec management" from Aug 13,
> 2018 (linux-next), leads to the following Smatch static checker
> warning:
> 
>          drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c:917 ixgbe_ipsec_vf_add_sa()
>          warn: sleeping in IRQ context
> 
> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>      890 int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
>      891 {
>      892         struct ixgbe_ipsec *ipsec = adapter->ipsec;
>      893         struct xfrm_algo_desc *algo;
>      894         struct sa_mbx_msg *sam;
>      895         struct xfrm_state *xs;
>      896         size_t aead_len;
>      897         u16 sa_idx;
>      898         u32 pfsa;
>      899         int err;
>      900
>      901         sam = (struct sa_mbx_msg *)(&msgbuf[1]);
>      902         if (!adapter->vfinfo[vf].trusted ||
>      903             !(adapter->flags2 & IXGBE_FLAG2_VF_IPSEC_ENABLED)) {
>      904                 e_warn(drv, "VF %d attempted to add an IPsec SA\n", vf);
>      905                 err = -EACCES;
>      906                 goto err_out;
>      907         }
>      908
>      909         /* Tx IPsec offload doesn't seem to work on this
>      910          * device, so block these requests for now.
>      911          */
>      912         if (sam->dir != XFRM_DEV_OFFLOAD_IN) {
>      913                 err = -EOPNOTSUPP;
>      914                 goto err_out;
>      915         }
>      916
> --> 917         xs = kzalloc(sizeof(*xs), GFP_KERNEL);
>                                            ^^^^^^^^^^
> Sleeping allocation.
> 
> The call tree that Smatch is worried about is:
> 
> ixgbe_msix_other() <- IRQ handler
> -> ixgbe_msg_task()
>     -> ixgbe_rcv_msg_from_vf()
>        -> ixgbe_ipsec_vf_add_sa()
> 
> This is a fairly new warning and those have a higher risk of false
> positives.  Plus the longer the call tree the higher the chance of
> false positives.  However, I did review it and the warning looks
> reasonable.
> 
> regards,
> dan carpenter

Hmmm... yes, this does look to be a valid issue.  Nothing like getting 
haunted by code from the past.  Thanks (?) for digging this up :-) .

I'm not sure offhand what the right answer might be.  I suppose choices 
include
   (a) pre-allocating some number of these xfrm_state structs
   (b) shoving the sa creation into a workthread
   (c) remove the VF xfrm offload feature
Neither of these options seem very appetizing.

I would guess that (b) is the "correct" answer, but I don't know how 
well the PF<->VF mailbox protocol can tolerate the need for a delayed 
response - it looks like the PF's handler wants to send an immediate 
ACK/NACK.

The pre-allocations for choice (a) would allow for not messing with the 
timing of the result message, but would require guessing at how many 744 
byte xfrm_state structs should be lying around for potential use.  The 
device has 1k slots available, but I don't think we want to store up 
that many nearly 1k structs that likely won't be used.  Maybe add a 
switch in the PF for enabling this, which defaults to off?

Meanwhile, (c) is the quick and dirty answer for a feature that likely 
doesn't see much use (I have no data for this assertion, just a guess), 
and shouldn't be relied upon anyway.

I'm not in a position at the moment to be able to address this issue, 
but I'm happy to try to answer questions for anyone who can get to it. 
I'm hoping that Jesse, Jake, or Tony might have a better idea what to do 
with this.

Thanks,
sln

Re: [Intel-wired-lan] [bug report] ixgbe: add VF IPsec management

[Przemek Kitszel]

On 2/9/24 18:57, Nelson, Shannon wrote:
> On 2/9/2024 4:59 AM, Dan Carpenter wrote:
>>
>> Hello Shannon Nelson,
>>
>> The patch eda0333ac293: "ixgbe: add VF IPsec management" from Aug 13,
>> 2018 (linux-next), leads to the following Smatch static checker
>> warning:
>>
>>          drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c:917 
>> ixgbe_ipsec_vf_add_sa()
>>          warn: sleeping in IRQ context
>>
>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>      890 int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, u32 
>> *msgbuf, u32 vf)
>>      891 {
>>      892         struct ixgbe_ipsec *ipsec = adapter->ipsec;
>>      893         struct xfrm_algo_desc *algo;
>>      894         struct sa_mbx_msg *sam;
>>      895         struct xfrm_state *xs;
>>      896         size_t aead_len;
>>      897         u16 sa_idx;
>>      898         u32 pfsa;
>>      899         int err;
>>      900
>>      901         sam = (struct sa_mbx_msg *)(&msgbuf[1]);
>>      902         if (!adapter->vfinfo[vf].trusted ||
>>      903             !(adapter->flags2 & IXGBE_FLAG2_VF_IPSEC_ENABLED)) {
>>      904                 e_warn(drv, "VF %d attempted to add an IPsec 
>> SA\n", vf);
>>      905                 err = -EACCES;
>>      906                 goto err_out;
>>      907         }
>>      908
>>      909         /* Tx IPsec offload doesn't seem to work on this
>>      910          * device, so block these requests for now.
>>      911          */
>>      912         if (sam->dir != XFRM_DEV_OFFLOAD_IN) {
>>      913                 err = -EOPNOTSUPP;
>>      914                 goto err_out;
>>      915         }
>>      916
>> --> 917         xs = kzalloc(sizeof(*xs), GFP_KERNEL);
>>                                            ^^^^^^^^^^
>> Sleeping allocation.

what about using GFP_ATOMIC instead of the "default" GFP_KERNEL?
that would be quickest fix possible, not sure how often such
alloc would fail

>>
>> The call tree that Smatch is worried about is:
>>
>> ixgbe_msix_other() <- IRQ handler
>> -> ixgbe_msg_task()
>>     -> ixgbe_rcv_msg_from_vf()
>>        -> ixgbe_ipsec_vf_add_sa()
>>
>> This is a fairly new warning and those have a higher risk of false
>> positives.  Plus the longer the call tree the higher the chance of
>> false positives.  However, I did review it and the warning looks
>> reasonable.
>>
>> regards,
>> dan carpenter
> 
> Hmmm... yes, this does look to be a valid issue.  Nothing like getting 
> haunted by code from the past.  Thanks (?) for digging this up :-) .

:)

> 
> I'm not sure offhand what the right answer might be.  I suppose choices 
> include
>    (a) pre-allocating some number of these xfrm_state structs
>    (b) shoving the sa creation into a workthread
>    (c) remove the VF xfrm offload feature
> Neither of these options seem very appetizing.
> 
> I would guess that (b) is the "correct" answer, but I don't know how 
> well the PF<->VF mailbox protocol can tolerate the need for a delayed 
> response - it looks like the PF's handler wants to send an immediate 
> ACK/NACK.
> 
> The pre-allocations for choice (a) would allow for not messing with the 
> timing of the result message, but would require guessing at how many 744 
> byte xfrm_state structs should be lying around for potential use.  The 
> device has 1k slots available, but I don't think we want to store up 
> that many nearly 1k structs that likely won't be used.  Maybe add a 
> switch in the PF for enabling this, which defaults to off?
> 
> Meanwhile, (c) is the quick and dirty answer for a feature that likely 
> doesn't see much use (I have no data for this assertion, just a guess), 
> and shouldn't be relied upon anyway.
> 
> I'm not in a position at the moment to be able to address this issue, 
> but I'm happy to try to answer questions for anyone who can get to it. 
> I'm hoping that Jesse, Jake, or Tony might have a better idea what to do 
> with this.
> 
> Thanks,
> sln
> 

Re: [Intel-wired-lan] [bug report] ixgbe: add VF IPsec management

[Nelson, Shannon]

On 2/14/2024 5:58 AM, Przemek Kitszel wrote:
> 
> On 2/9/24 18:57, Nelson, Shannon wrote:
>> On 2/9/2024 4:59 AM, Dan Carpenter wrote:
>>>
>>> Hello Shannon Nelson,
>>>
>>> The patch eda0333ac293: "ixgbe: add VF IPsec management" from Aug 13,
>>> 2018 (linux-next), leads to the following Smatch static checker
>>> warning:
>>>
>>>          drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c:917
>>> ixgbe_ipsec_vf_add_sa()
>>>          warn: sleeping in IRQ context
>>>
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>>      890 int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, u32
>>> *msgbuf, u32 vf)
>>>      891 {
>>>      892         struct ixgbe_ipsec *ipsec = adapter->ipsec;
>>>      893         struct xfrm_algo_desc *algo;
>>>      894         struct sa_mbx_msg *sam;
>>>      895         struct xfrm_state *xs;
>>>      896         size_t aead_len;
>>>      897         u16 sa_idx;
>>>      898         u32 pfsa;
>>>      899         int err;
>>>      900
>>>      901         sam = (struct sa_mbx_msg *)(&msgbuf[1]);
>>>      902         if (!adapter->vfinfo[vf].trusted ||
>>>      903             !(adapter->flags2 & 
>>> IXGBE_FLAG2_VF_IPSEC_ENABLED)) {
>>>      904                 e_warn(drv, "VF %d attempted to add an IPsec
>>> SA\n", vf);
>>>      905                 err = -EACCES;
>>>      906                 goto err_out;
>>>      907         }
>>>      908
>>>      909         /* Tx IPsec offload doesn't seem to work on this
>>>      910          * device, so block these requests for now.
>>>      911          */
>>>      912         if (sam->dir != XFRM_DEV_OFFLOAD_IN) {
>>>      913                 err = -EOPNOTSUPP;
>>>      914                 goto err_out;
>>>      915         }
>>>      916
>>> --> 917         xs = kzalloc(sizeof(*xs), GFP_KERNEL);
>>>                                            ^^^^^^^^^^
>>> Sleeping allocation.
> 
> what about using GFP_ATOMIC instead of the "default" GFP_KERNEL?
> that would be quickest fix possible, not sure how often such
> alloc would fail

That certainly is another quick option, and may be perfectly reasonable. 
  Can you run with it?

sln

> 
>>>
>>> The call tree that Smatch is worried about is:
>>>
>>> ixgbe_msix_other() <- IRQ handler
>>> -> ixgbe_msg_task()
>>>     -> ixgbe_rcv_msg_from_vf()
>>>        -> ixgbe_ipsec_vf_add_sa()
>>>
>>> This is a fairly new warning and those have a higher risk of false
>>> positives.  Plus the longer the call tree the higher the chance of
>>> false positives.  However, I did review it and the warning looks
>>> reasonable.
>>>
>>> regards,
>>> dan carpenter
>>
>> Hmmm... yes, this does look to be a valid issue.  Nothing like getting
>> haunted by code from the past.  Thanks (?) for digging this up :-) .
> 
> :)
> 
>>
>> I'm not sure offhand what the right answer might be.  I suppose choices
>> include
>>    (a) pre-allocating some number of these xfrm_state structs
>>    (b) shoving the sa creation into a workthread
>>    (c) remove the VF xfrm offload feature
>> Neither of these options seem very appetizing.
>>
>> I would guess that (b) is the "correct" answer, but I don't know how
>> well the PF<->VF mailbox protocol can tolerate the need for a delayed
>> response - it looks like the PF's handler wants to send an immediate
>> ACK/NACK.
>>
>> The pre-allocations for choice (a) would allow for not messing with the
>> timing of the result message, but would require guessing at how many 744
>> byte xfrm_state structs should be lying around for potential use.  The
>> device has 1k slots available, but I don't think we want to store up
>> that many nearly 1k structs that likely won't be used.  Maybe add a
>> switch in the PF for enabling this, which defaults to off?
>>
>> Meanwhile, (c) is the quick and dirty answer for a feature that likely
>> doesn't see much use (I have no data for this assertion, just a guess),
>> and shouldn't be relied upon anyway.
>>
>> I'm not in a position at the moment to be able to address this issue,
>> but I'm happy to try to answer questions for anyone who can get to it.
>> I'm hoping that Jesse, Jake, or Tony might have a better idea what to do
>> with this.
>>
>> Thanks,
>> sln
>>
>