diff mbox series

[1/3] xfrm: Use the XFRM_GRO to indicate a GRO call on input.

Message ID 6dfd03c5fa0afb99f255f4a35772df19e33880db.1674156645.git.antony.antony@secunet.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series [1/3] xfrm: Use the XFRM_GRO to indicate a GRO call on input. | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter warning Series does not have a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/cc_maintainers warning 6 maintainers not CCed: yoshfuji@linux-ipv6.org edumazet@google.com davem@davemloft.net kuba@kernel.org dsahern@kernel.org pabeni@redhat.com
netdev/build_clang success Errors and warnings before: 1 this patch: 1
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 5 this patch: 5
netdev/checkpatch warning WARNING: line length of 86 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Antony Antony Jan. 19, 2023, 7:33 p.m. UTC 
From: Steffen Klassert <steffen.klassert@secunet.com>

This is needed to support GRO for ESP in UDP encapsulation.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
 net/ipv4/esp4_offload.c |  2 +-
 net/ipv6/esp6_offload.c |  2 +-
 net/xfrm/xfrm_input.c   | 75 +++++++++++++++++++++++------------------
 3 files changed, 44 insertions(+), 35 deletions(-)

Comments

Eyal Birger Jan. 20, 2023, 11:05 a.m. UTC | #1 
Hi,

On Thu, Jan 19, 2023 at 9:59 PM Antony Antony <antony.antony@secunet.com> wrote:
>
> From: Steffen Klassert <steffen.klassert@secunet.com>
>
> This is needed to support GRO for ESP in UDP encapsulation.
>
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> Signed-off-by: Antony Antony <antony.antony@secunet.com>
> ---
>  net/ipv4/esp4_offload.c |  2 +-
>  net/ipv6/esp6_offload.c |  2 +-
>  net/xfrm/xfrm_input.c   | 75 +++++++++++++++++++++++------------------
>  3 files changed, 44 insertions(+), 35 deletions(-)
>
> diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
> index 3969fa805679..77bb01032667 100644
> --- a/net/ipv4/esp4_offload.c
> +++ b/net/ipv4/esp4_offload.c
> @@ -76,7 +76,7 @@ static struct sk_buff *esp4_gro_receive(struct list_head *head,
>
>         /* We don't need to handle errors from xfrm_input, it does all
>          * the error handling and frees the resources on error. */
> -       xfrm_input(skb, IPPROTO_ESP, spi, -2);
> +       xfrm_input(skb, IPPROTO_ESP, spi, 0);
>
>         return ERR_PTR(-EINPROGRESS);
>  out_reset:
> diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c
> index 75c02992c520..ee5f5abdb503 100644
> --- a/net/ipv6/esp6_offload.c
> +++ b/net/ipv6/esp6_offload.c
> @@ -103,7 +103,7 @@ static struct sk_buff *esp6_gro_receive(struct list_head *head,
>
>         /* We don't need to handle errors from xfrm_input, it does all
>          * the error handling and frees the resources on error. */
> -       xfrm_input(skb, IPPROTO_ESP, spi, -2);
> +       xfrm_input(skb, IPPROTO_ESP, spi, 0);
>
>         return ERR_PTR(-EINPROGRESS);
>  out_reset:
> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index c06e54a10540..ffd62ad58207 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -458,6 +458,35 @@ static int xfrm_inner_mode_input(struct xfrm_state *x,
>         return -EOPNOTSUPP;
>  }
>
> +static int xfrm_input_check_offload(struct net *net, struct sk_buff *skb,
> +                                   struct xfrm_state *x,
> +                                   struct xfrm_offload *xo)
> +{
> +       if (!(xo->status & CRYPTO_SUCCESS)) {
> +               if (xo->status &
> +                   (CRYPTO_TRANSPORT_AH_AUTH_FAILED |
> +                    CRYPTO_TRANSPORT_ESP_AUTH_FAILED |
> +                    CRYPTO_TUNNEL_AH_AUTH_FAILED |
> +                    CRYPTO_TUNNEL_ESP_AUTH_FAILED)) {
> +                       xfrm_audit_state_icvfail(x, skb,
> +                                                x->type->proto);
> +                       x->stats.integrity_failed++;
> +                       XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEPROTOERROR);
> +                       return -EINVAL;
> +               }
> +
> +               if (xo->status & CRYPTO_INVALID_PROTOCOL) {
> +                       XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEPROTOERROR);
> +                       return -EINVAL;
> +               }
> +
> +               XFRM_INC_STATS(net, LINUX_MIB_XFRMINBUFFERERROR);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
>  int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  {
>         const struct xfrm_state_afinfo *afinfo;
> @@ -477,7 +506,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>         struct xfrm_offload *xo = xfrm_offload(skb);
>         struct sec_path *sp;
>
> -       if (encap_type < 0) {
> +       if (encap_type < 0 || (xo && xo->flags & XFRM_GRO)) {
>                 x = xfrm_input_state(skb);
>
>                 if (unlikely(x->km.state != XFRM_STATE_VALID)) {
> @@ -495,46 +524,26 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>                 family = x->outer_mode.family;
>
>                 /* An encap_type of -1 indicates async resumption. */
> -               if (encap_type == -1) {
> +               if (encap_type  < 0) {

Why is this specific line change needed? I see that now -2 is not sent
anymore, so how is this related?
If it is needed, maybe the comment above also needs updating?

nit, a cover letter would've been handy so that the series could be
fetched as a whole.

Eyal.
Antony Antony Sept. 26, 2023, 10:14 a.m. UTC | #2 
Hi,

I have rebased the patch set to latest ipsec-next. There was a big change to udp socket encapsulation data structure.

Eyal, would please review patch set quickly? focus specifically chages due to

70a36f571362 ("udp: annotate data-races around udp->encap_type")
ac9a7f4ce5dd ("udp: lockless UDP_ENCAP_L2TPINUDP / UDP_GRO")
I hope I incorprated these changes correctly.

v1->v2 fixed error path added skb_push
        use is_fou instead of holding sk in skb.
        user configurable option to enable GRO; using UDP_GRO

v2->v3 only support GRO for UDP_ENCAP_ESPINUDP and not
        UDP_ENCAP_ESPINUDP_NON_IKE. The _NON_IKE is an IETF early draft
        version and not widly used.

v3->v4 removed refactoring since refactored function is only used once
        removed refcount on sk, sk is not used any more.
        fixed encap_type as Eyal recommended.
        removed un-necessary else since there is a goto before that.

v4->v5 removed extra code/checks that accidently got added.

v5->v6 rebased to ipsec-next chages due lockless scket udp
       encapsulation options

Steffen Klassert (3):
  xfrm: Use the XFRM_GRO to indicate a GRO call on input
  xfrm: Support GRO for IPv4 ESP in UDP encapsulation
  xfrm: Support GRO for IPv6 ESP in UDP encapsulation

 include/net/gro.h        |  2 +-
 include/net/ipv6_stubs.h |  3 ++
 include/net/xfrm.h       |  4 ++
 net/ipv4/esp4_offload.c  |  6 ++-
 net/ipv4/udp.c           | 16 +++++++
 net/ipv4/xfrm4_input.c   | 94 ++++++++++++++++++++++++++++++++--------
 net/ipv6/af_inet6.c      |  1 +
 net/ipv6/esp6_offload.c  | 10 ++++-
 net/ipv6/xfrm6_input.c   | 94 ++++++++++++++++++++++++++++++++--------
 net/xfrm/xfrm_input.c    |  6 +--
 10 files changed, 192 insertions(+), 44 deletions(-)

--
2.30.2
Eyal Birger Sept. 26, 2023, 1:07 p.m. UTC | #3 
Hi Antony,

On Tue, Sep 26, 2023 at 1:14 PM Antony Antony <antony.antony@secunet.com> wrote:
>
> Hi,
>
> I have rebased the patch set to latest ipsec-next. There was a big change to udp socket encapsulation data structure.
>
> Eyal, would please review patch set quickly? focus specifically chages due to
>
> 70a36f571362 ("udp: annotate data-races around udp->encap_type")
> ac9a7f4ce5dd ("udp: lockless UDP_ENCAP_L2TPINUDP / UDP_GRO")
> I hope I incorprated these changes correctly.

LGTM.

I think a cover letter explaining the feature, usage, performance,
caveats etc, would be helpful.

For the series:

Reviewed-by: Eyal Birger <eyal.birger@gmail.com>

>
> v1->v2 fixed error path added skb_push
>         use is_fou instead of holding sk in skb.
>         user configurable option to enable GRO; using UDP_GRO
>
> v2->v3 only support GRO for UDP_ENCAP_ESPINUDP and not
>         UDP_ENCAP_ESPINUDP_NON_IKE. The _NON_IKE is an IETF early draft
>         version and not widly used.
>
> v3->v4 removed refactoring since refactored function is only used once
>         removed refcount on sk, sk is not used any more.
>         fixed encap_type as Eyal recommended.
>         removed un-necessary else since there is a goto before that.
>
> v4->v5 removed extra code/checks that accidently got added.
>
> v5->v6 rebased to ipsec-next chages due lockless scket udp
>        encapsulation options
>
> Steffen Klassert (3):
>   xfrm: Use the XFRM_GRO to indicate a GRO call on input
>   xfrm: Support GRO for IPv4 ESP in UDP encapsulation
>   xfrm: Support GRO for IPv6 ESP in UDP encapsulation
>
>  include/net/gro.h        |  2 +-
>  include/net/ipv6_stubs.h |  3 ++
>  include/net/xfrm.h       |  4 ++
>  net/ipv4/esp4_offload.c  |  6 ++-
>  net/ipv4/udp.c           | 16 +++++++
>  net/ipv4/xfrm4_input.c   | 94 ++++++++++++++++++++++++++++++++--------
>  net/ipv6/af_inet6.c      |  1 +
>  net/ipv6/esp6_offload.c  | 10 ++++-
>  net/ipv6/xfrm6_input.c   | 94 ++++++++++++++++++++++++++++++++--------
>  net/xfrm/xfrm_input.c    |  6 +--
>  10 files changed, 192 insertions(+), 44 deletions(-)
>
> --
> 2.30.2
>
diff mbox series

Patch

diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
index 3969fa805679..77bb01032667 100644
--- a/net/ipv4/esp4_offload.c
+++ b/net/ipv4/esp4_offload.c
@@ -76,7 +76,7 @@  static struct sk_buff *esp4_gro_receive(struct list_head *head,
 
 	/* We don't need to handle errors from xfrm_input, it does all
 	 * the error handling and frees the resources on error. */
-	xfrm_input(skb, IPPROTO_ESP, spi, -2);
+	xfrm_input(skb, IPPROTO_ESP, spi, 0);
 
 	return ERR_PTR(-EINPROGRESS);
 out_reset:
diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c
index 75c02992c520..ee5f5abdb503 100644
--- a/net/ipv6/esp6_offload.c
+++ b/net/ipv6/esp6_offload.c
@@ -103,7 +103,7 @@  static struct sk_buff *esp6_gro_receive(struct list_head *head,
 
 	/* We don't need to handle errors from xfrm_input, it does all
 	 * the error handling and frees the resources on error. */
-	xfrm_input(skb, IPPROTO_ESP, spi, -2);
+	xfrm_input(skb, IPPROTO_ESP, spi, 0);
 
 	return ERR_PTR(-EINPROGRESS);
 out_reset:
diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index c06e54a10540..ffd62ad58207 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -458,6 +458,35 @@  static int xfrm_inner_mode_input(struct xfrm_state *x,
 	return -EOPNOTSUPP;
 }
 
+static int xfrm_input_check_offload(struct net *net, struct sk_buff *skb,
+				    struct xfrm_state *x,
+				    struct xfrm_offload *xo)
+{
+	if (!(xo->status & CRYPTO_SUCCESS)) {
+		if (xo->status &
+		    (CRYPTO_TRANSPORT_AH_AUTH_FAILED |
+		     CRYPTO_TRANSPORT_ESP_AUTH_FAILED |
+		     CRYPTO_TUNNEL_AH_AUTH_FAILED |
+		     CRYPTO_TUNNEL_ESP_AUTH_FAILED)) {
+			xfrm_audit_state_icvfail(x, skb,
+						 x->type->proto);
+			x->stats.integrity_failed++;
+			XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEPROTOERROR);
+			return -EINVAL;
+		}
+
+		if (xo->status & CRYPTO_INVALID_PROTOCOL) {
+			XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEPROTOERROR);
+			return -EINVAL;
+		}
+
+		XFRM_INC_STATS(net, LINUX_MIB_XFRMINBUFFERERROR);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 {
 	const struct xfrm_state_afinfo *afinfo;
@@ -477,7 +506,7 @@  int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	struct xfrm_offload *xo = xfrm_offload(skb);
 	struct sec_path *sp;
 
-	if (encap_type < 0) {
+	if (encap_type < 0 || (xo && xo->flags & XFRM_GRO)) {
 		x = xfrm_input_state(skb);
 
 		if (unlikely(x->km.state != XFRM_STATE_VALID)) {
@@ -495,46 +524,26 @@  int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 		family = x->outer_mode.family;
 
 		/* An encap_type of -1 indicates async resumption. */
-		if (encap_type == -1) {
+		if (encap_type  < 0) {
 			async = 1;
 			seq = XFRM_SKB_CB(skb)->seq.input.low;
 			goto resume;
-		}
+		} else {
+			/* GRO call */
+			seq = XFRM_SPI_SKB_CB(skb)->seq;
 
-		/* encap_type < -1 indicates a GRO call. */
-		encap_type = 0;
-		seq = XFRM_SPI_SKB_CB(skb)->seq;
-
-		if (xo && (xo->flags & CRYPTO_DONE)) {
-			crypto_done = true;
-			family = XFRM_SPI_SKB_CB(skb)->family;
-
-			if (!(xo->status & CRYPTO_SUCCESS)) {
-				if (xo->status &
-				    (CRYPTO_TRANSPORT_AH_AUTH_FAILED |
-				     CRYPTO_TRANSPORT_ESP_AUTH_FAILED |
-				     CRYPTO_TUNNEL_AH_AUTH_FAILED |
-				     CRYPTO_TUNNEL_ESP_AUTH_FAILED)) {
-
-					xfrm_audit_state_icvfail(x, skb,
-								 x->type->proto);
-					x->stats.integrity_failed++;
-					XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEPROTOERROR);
+			if (xo && (xo->flags & CRYPTO_DONE)) {
+				crypto_done = true;
+				family = XFRM_SPI_SKB_CB(skb)->family;
+
+				err = xfrm_input_check_offload(net, skb, x, xo);
+				if (err)
 					goto drop;
-				}
 
-				if (xo->status & CRYPTO_INVALID_PROTOCOL) {
-					XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEPROTOERROR);
+				if (xfrm_parse_spi(skb, nexthdr, &spi, &seq)) {
+					XFRM_INC_STATS(net, LINUX_MIB_XFRMINHDRERROR);
 					goto drop;
 				}
-
-				XFRM_INC_STATS(net, LINUX_MIB_XFRMINBUFFERERROR);
-				goto drop;
-			}
-
-			if (xfrm_parse_spi(skb, nexthdr, &spi, &seq)) {
-				XFRM_INC_STATS(net, LINUX_MIB_XFRMINHDRERROR);
-				goto drop;
 			}
 		}