[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240829103846.GE26654@unreal>
Date: Thu, 29 Aug 2024 13:38:46 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Feng Wang <wangfe@...gle.com>
Cc: Steffen Klassert <steffen.klassert@...unet.com>, netdev@...r.kernel.org,
antony.antony@...unet.com
Subject: Re: [PATCH] xfrm: add SA information to the offloaded packet
On Wed, Aug 28, 2024 at 02:25:22PM -0700, Feng Wang wrote:
> Hi Leon,
Please don't top-post your replies when you are replying to a mailing
list. It makes it hard to follow the conversation.
>
> Thank you for your insightful questions and comments.
>
> Just like in crypto offload mode, now pass SA (Security Association)
> information to the driver in packet offload mode. This helps the
> driver quickly identify the packet's source and destination, rather
> than having to parse the packet itself. The xfrm interface ID is
> especially useful here. As you can see in the kernel code
> (https://elixir.bootlin.com/linux/v6.10/source/net/xfrm/xfrm_policy.c#L1993),
> it's used to differentiate between various policies in complex network
> setups.
Which in-kernel driver use this information?
>
> During my testing of packet offload mode without the patch, I observed
> that the sec_path was NULL. Consequently, xo was also NULL when
> validate_xmit_xfrm was called, leading to an immediate return at line
> 125 (https://elixir.bootlin.com/linux/v6.10/source/net/xfrm/xfrm_device.c#L125).
It is intentional, because the packet is forwarded to HW as plain text
and it is not offloaded (doesn't have xfrm_offload()).
>
> Regarding the potential xfrm_state leak, upon further investigation,
> it may not be the case. It seems that both secpath_reset and kfree_skb
> invoke the xfrm_state_put function, which should be responsible for
> releasing the state. The call flow appears to be as follows: kfree_skb
> -> skb_release_all -> skb_release_head_state -> skb_ext_put ->
> skb_ext_put_sp -> xfrm_state_put.
You are trying to make same flow as for crypto, but it is not the same,
in crypto case secpath_reset() was called to release SKB extensions and
perform cleanup, first and only after that new xfrm_state_hold() was
called, but in new code SKB is not reset.
Thanks
>
> Please let me know if you have any further questions or concerns. I
> appreciate your valuable feedback!
>
> Feng
>
> On Wed, Aug 28, 2024 at 4:26 AM Leon Romanovsky <leon@...nel.org> wrote:
> >
> > On Wed, Aug 28, 2024 at 07:32:47AM +0200, Steffen Klassert wrote:
> > > On Thu, Aug 22, 2024 at 01:02:52PM -0700, Feng Wang wrote:
> > > > From: wangfe <wangfe@...gle.com>
> > > >
> > > > In packet offload mode, append Security Association (SA) information
> > > > to each packet, replicating the crypto offload implementation.
> > > > The XFRM_XMIT flag is set to enable packet to be returned immediately
> > > > from the validate_xmit_xfrm function, thus aligning with the existing
> > > > code path for packet offload mode.
> > > >
> > > > This SA info helps HW offload match packets to their correct security
> > > > policies. The XFRM interface ID is included, which is crucial in setups
> > > > with multiple XFRM interfaces where source/destination addresses alone
> > > > can't pinpoint the right policy.
> > > >
> > > > Signed-off-by: wangfe <wangfe@...gle.com>
> > >
> > > Applied to ipsec-next, thanks Feng!
> >
> > Stephen, can you please explain why do you think that this is correct
> > thing to do?
> >
> > There are no in-tree any drivers which is using this information, and it
> > is unclear to me how state is released and it has controversial code
> > around validity of xfrm_offload() too.
> >
> > For example:
> > + sp->olen++;
> > + sp->xvec[sp->len++] = x;
> > + xfrm_state_hold(x);
> > +
> > + xo = xfrm_offload(skb);
> > + if (!xo) { <--- previous code handled this case perfectly in validate_xmit_xfrm
> > + secpath_reset(skb);
> > + XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> > + kfree_skb(skb);
> > + return -EINVAL; <--- xfrm state leak
> > + }
> >
> >
> > Can you please revert/drop this patch for now?
> >
> > Thanks
Powered by blists - more mailing lists