Re: [PATCH xfrm-next v2 4/6] xfrm: add TX datapath support for IPsec full offload mode

[Leon Romanovsky <leon@kernel.org>]

On Thu, Aug 18, 2022 at 12:24:51PM +0200, Steffen Klassert wrote:
> On Tue, Aug 16, 2022 at 11:59:25AM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > In IPsec full mode, the device is going to encrypt and encapsulate
> > packets that are associated with offloaded policy. After successful
> > policy lookup to indicate if packets should be offloaded or not,
> > the stack forwards packets to the device to do the magic.
> > 
> > Signed-off-by: Raed Salem <raeds@nvidia.com>
> > Signed-off-by: Huy Nguyen <huyn@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  net/xfrm/xfrm_device.c | 13 +++++++++++++
> >  net/xfrm/xfrm_output.c | 20 ++++++++++++++++++++
> >  2 files changed, 33 insertions(+)
> > 
> > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> > index 1cc482e9c87d..db5ebd36f68c 100644
> > --- a/net/xfrm/xfrm_device.c
> > +++ b/net/xfrm/xfrm_device.c
> 
> > @@ -366,6 +376,9 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
> >  	struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
> >  	struct net_device *dev = x->xso.dev;
> >  
> > +	if (x->xso.type == XFRM_DEV_OFFLOAD_FULL)
> > +		goto ok;
> 
> You skip the PMTU checks here. I've seen that you check
> the packet length against the device MTU in one of your
> mlx5 patches, but that does not help if the PMTU is below.

If device supports transformation of the packet, this packet
won't be counted as XFRM anymore. I'm not sure that we need
to perform XFRM specific checks.

> 
> >  
> > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> > index 555ab35cd119..27a8dac9ca7d 100644
> > --- a/net/xfrm/xfrm_output.c
> > +++ b/net/xfrm/xfrm_output.c
> > @@ -719,6 +719,26 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
> >  		break;
> >  	}
> >  
> > +	if (x->xso.type == XFRM_DEV_OFFLOAD_FULL) {
> > +		struct dst_entry *dst = skb_dst_pop(skb);
> > +
> > +		if (!dst || !xfrm_dev_offload_ok(skb, x)) {
> > +			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> > +			kfree_skb(skb);
> > +			return -EHOSTUNREACH;
> > +		}
> > +
> > +		skb_dst_set(skb, dst);
> > +		err = skb_dst(skb)->ops->local_out(net, skb->sk, skb);
> > +		if (unlikely(err != 1))
> > +			return err;
> > +
> > +		if (!skb_dst(skb)->xfrm)
> > +			return dst_output(net, skb->sk, skb);
> > +
> > +		return 0;
> 
> You leak skb here. Also, this skb needs another tfm because
> skb_dst(skb)->xfrm is set.

I will fix, thanks.

>