When using GSO it can happen that the wrong seq_hi is used for the last
packets before the wrap around. This can lead to double usage of a
sequence number. To avoid this, we should serialize this last GSO
packet.
Fixes: d7dbefc45cf5 ("xfrm: Add xfrm_replay_overflow functions for offloading")
Co-developed-by: Steffen Klassert <[email protected]>
Signed-off-by: Christian Langrock <[email protected]>
---
Changes in v6:
- move overflow check to offloading path to avoid locking issues
Changes in v5:
- Fix build
Changes in v4:
- move changelog within comment
- add reviewer
Changes in v3:
- fix build
- remove wrapper function
Changes in v2:
- switch to bool as return value
- remove switch case in wrapper function
---
net/ipv4/esp4_offload.c | 3 +++
net/ipv6/esp6_offload.c | 3 +++
net/xfrm/xfrm_device.c | 15 ++++++++++++++-
net/xfrm/xfrm_replay.c | 2 +-
4 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
index 170152772d33..3969fa805679 100644
--- a/net/ipv4/esp4_offload.c
+++ b/net/ipv4/esp4_offload.c
@@ -314,6 +314,9 @@ static int esp_xmit(struct xfrm_state *x, struct sk_buff *skb, netdev_features_
xo->seq.low += skb_shinfo(skb)->gso_segs;
}
+ if (xo->seq.low < seq)
+ xo->seq.hi++;
+
esp.seqno = cpu_to_be64(seq + ((u64)xo->seq.hi << 32));
ip_hdr(skb)->tot_len = htons(skb->len);
diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c
index 79d43548279c..242f4295940e 100644
--- a/net/ipv6/esp6_offload.c
+++ b/net/ipv6/esp6_offload.c
@@ -346,6 +346,9 @@ static int esp6_xmit(struct xfrm_state *x, struct sk_buff *skb, netdev_features
xo->seq.low += skb_shinfo(skb)->gso_segs;
}
+ if (xo->seq.low < seq)
+ xo->seq.hi++;
+
esp.seqno = cpu_to_be64(xo->seq.low + ((u64)xo->seq.hi << 32));
len = skb->len - sizeof(struct ipv6hdr);
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 5f5aafd418af..21269e8f2db4 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -97,6 +97,18 @@ static void xfrm_outer_mode_prep(struct xfrm_state *x, struct sk_buff *skb)
}
}
+static inline bool xmit_xfrm_check_overflow(struct sk_buff *skb)
+{
+ struct xfrm_offload *xo = xfrm_offload(skb);
+ __u32 seq = xo->seq.low;
+
+ seq += skb_shinfo(skb)->gso_segs;
+ if (unlikely(seq < xo->seq.low))
+ return true;
+
+ return false;
+}
+
struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t features, bool *again)
{
int err;
@@ -134,7 +146,8 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur
return skb;
}
- if (skb_is_gso(skb) && unlikely(x->xso.dev != dev)) {
+ if (skb_is_gso(skb) && (unlikely(x->xso.dev != dev) ||
+ unlikely(xmit_xfrm_check_overflow(skb)))) {
struct sk_buff *segs;
/* Packet got rerouted, fixup features and segment it. */
diff --git a/net/xfrm/xfrm_replay.c b/net/xfrm/xfrm_replay.c
index 9f4d42eb090f..ce56d659c55a 100644
--- a/net/xfrm/xfrm_replay.c
+++ b/net/xfrm/xfrm_replay.c
@@ -714,7 +714,7 @@ static int xfrm_replay_overflow_offload_esn(struct xfrm_state *x, struct sk_buff
oseq += skb_shinfo(skb)->gso_segs;
}
- if (unlikely(oseq < replay_esn->oseq)) {
+ if (unlikely(xo->seq.low < replay_esn->oseq)) {
XFRM_SKB_CB(skb)->seq.output.hi = ++oseq_hi;
xo->seq.hi = oseq_hi;
replay_esn->oseq_hi = oseq_hi;
base-commit: 0326074ff4652329f2a1a9c8685104576bd8d131
--
2.37.1.223.g6a475b71f8
On Fri, Oct 07, 2022 at 04:50:15PM +0200, Christian Langrock wrote:
> When using GSO it can happen that the wrong seq_hi is used for the last
> packets before the wrap around. This can lead to double usage of a
> sequence number. To avoid this, we should serialize this last GSO
> packet.
>
> Fixes: d7dbefc45cf5 ("xfrm: Add xfrm_replay_overflow functions for offloading")
> Co-developed-by: Steffen Klassert <[email protected]>
> Signed-off-by: Christian Langrock <[email protected]>
> ---
> Changes in v6:
> - move overflow check to offloading path to avoid locking issues
>
> Changes in v5:
> - Fix build
>
> Changes in v4:
> - move changelog within comment
> - add reviewer
>
> Changes in v3:
> - fix build
> - remove wrapper function
>
> Changes in v2:
> - switch to bool as return value
> - remove switch case in wrapper function
> ---
> net/ipv4/esp4_offload.c | 3 +++
> net/ipv6/esp6_offload.c | 3 +++
> net/xfrm/xfrm_device.c | 15 ++++++++++++++-
> net/xfrm/xfrm_replay.c | 2 +-
> 4 files changed, 21 insertions(+), 2 deletions(-)
Your patch does not apply to the ipsec tree. Looks
like it is malformed by your mailer.
On Fri, Oct 07, 2022 at 04:50:15PM +0200, Christian Langrock wrote:
> When using GSO it can happen that the wrong seq_hi is used for the last
> packets before the wrap around. This can lead to double usage of a
> sequence number. To avoid this, we should serialize this last GSO
> packet.
>
> Fixes: d7dbefc45cf5 ("xfrm: Add xfrm_replay_overflow functions for offloading")
> Co-developed-by: Steffen Klassert <[email protected]>
> Signed-off-by: Christian Langrock <[email protected]>
> ---
> Changes in v6:
> - move overflow check to offloading path to avoid locking issues
>
> Changes in v5:
> - Fix build
>
> Changes in v4:
> - move changelog within comment
> - add reviewer
>
> Changes in v3:
> - fix build
> - remove wrapper function
>
> Changes in v2:
> - switch to bool as return value
> - remove switch case in wrapper function
> ---
> net/ipv4/esp4_offload.c | 3 +++
> net/ipv6/esp6_offload.c | 3 +++
> net/xfrm/xfrm_device.c | 15 ++++++++++++++-
> net/xfrm/xfrm_replay.c | 2 +-
> 4 files changed, 21 insertions(+), 2 deletions(-)
Could you please explain how this code restructure makes it safe
with respect to multiple users of the same xfrm_state?
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Wed, Oct 12, 2022 at 05:28:57PM +0800, Herbert Xu wrote:
> On Fri, Oct 07, 2022 at 04:50:15PM +0200, Christian Langrock wrote:
> > When using GSO it can happen that the wrong seq_hi is used for the last
> > packets before the wrap around. This can lead to double usage of a
> > sequence number. To avoid this, we should serialize this last GSO
> > packet.
> >
> > Fixes: d7dbefc45cf5 ("xfrm: Add xfrm_replay_overflow functions for offloading")
> > Co-developed-by: Steffen Klassert <[email protected]>
> > Signed-off-by: Christian Langrock <[email protected]>
> > ---
> > Changes in v6:
> > - move overflow check to offloading path to avoid locking issues
> >
> > Changes in v5:
> > - Fix build
> >
> > Changes in v4:
> > - move changelog within comment
> > - add reviewer
> >
> > Changes in v3:
> > - fix build
> > - remove wrapper function
> >
> > Changes in v2:
> > - switch to bool as return value
> > - remove switch case in wrapper function
> > ---
> > net/ipv4/esp4_offload.c | 3 +++
> > net/ipv6/esp6_offload.c | 3 +++
> > net/xfrm/xfrm_device.c | 15 ++++++++++++++-
> > net/xfrm/xfrm_replay.c | 2 +-
> > 4 files changed, 21 insertions(+), 2 deletions(-)
>
> Could you please explain how this code restructure makes it safe
> with respect to multiple users of the same xfrm_state?
That is because with this patch, the sequence number from the xfrm_state
is assigned to the skb and advanced by the number of segments while
holding the state lock, as it was before. The sequence numbers this
patch operates on are exclusive and private to that skb (and its
segments). The next skb will checkout the correct number from the
xfrm_state regardless on which cpu it comes.
On Thu, Oct 13, 2022 at 08:16:33AM +0200, Steffen Klassert wrote:
>
> That is because with this patch, the sequence number from the xfrm_state
> is assigned to the skb and advanced by the number of segments while
> holding the state lock, as it was before. The sequence numbers this
> patch operates on are exclusive and private to that skb (and its
> segments). The next skb will checkout the correct number from the
> xfrm_state regardless on which cpu it comes.
Thanks for the explanation Steffen.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt