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
