Support PTMU in tunnel mode for packet offload

From: Leon Romanovsky <[email protected]>

XFRM offload path is probed even if offload isn't needed at all. Let's make sure that x->type_offload pointer stays NULL for such path to reduce ambiguity.

Fixes 9d389d7f84bb ("xfrm: Add a xfrm type offload.")

Signed-off-by Leon Romanovsky <[email protected]>

---
 include/net/xfrm.h     | 12 +++++++++++-
 net/xfrm/xfrm_device.c | 14 +++++++++-----
 net/xfrm/xfrm_state.c  | 22 +++++++++-------------
 net/xfrm/xfrm_user.c   |  2 +-
 4 files changed, 30 insertions(+), 20 deletions(-)

include/net/xfrm.h +11 -1

index ed4b83696c77..28355a5be5b9 100644
@@ -464,6 +464,16 @@ struct xfrm_type_offload {
 
 int xfrm_register_type_offload(const struct xfrm_type_offload *type, unsigned short family);
 void xfrm_unregister_type_offload(const struct xfrm_type_offload *type, unsigned short family);
+const struct xfrm_type_offload *xfrm_get_type_offload(u8 proto,
+                                                     unsigned short family);
+static inline void xfrm_put_type_offload(const struct xfrm_type_offload *type)
+{
+       if (!type)
+               return;
+
+       module_put(type->owner);
+}
+
 
 /**
  * struct xfrm_mode_cbs - XFRM mode callbacks

@@ -1760,7 +1770,7 @@ void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
 u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
 int xfrm_init_replay(struct xfrm_state *x, struct netlink_ext_ack *extack);
 u32 xfrm_state_mtu(struct xfrm_state *x, int mtu);
-int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload,
+int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
                      struct netlink_ext_ack *extack);
 int xfrm_init_state(struct xfrm_state *x);
 int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type);

net/xfrm/xfrm_device.c +7 -5

index d1fa94e52cea..e01a7f5a4c75 100644
@@ -244,11 +244,6 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
        xfrm_address_t *daddr;
        bool is_packet_offload;
 
-       if (!x->type_offload) {
-               NL_SET_ERR_MSG(extack, "Type doesn't support offload");
-               return -EINVAL;
-       }
-
        if (xuo->flags &
            ~(XFRM_OFFLOAD_IPV6 | XFRM_OFFLOAD_INBOUND | XFRM_OFFLOAD_PACKET)) {
                NL_SET_ERR_MSG(extack, "Unrecognized flags in offload request");

@@ -310,6 +305,13 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
                return -EINVAL;
        }
 
+       x->type_offload = xfrm_get_type_offload(x->id.proto, x->props.family);
+       if (!x->type_offload) {
+               NL_SET_ERR_MSG(extack, "Type doesn't support offload");
+               dev_put(dev);
+               return -EINVAL;
+       }
+
        xso->dev = dev;
        netdev_tracker_alloc(dev, &xso->dev_tracker, GFP_ATOMIC);
        xso->real_dev = dev;

net/xfrm/xfrm_device.c +0 -0

                /* User explicitly requested packet offload mode and configured
                 * policy in addition to the XFRM state. So be civil to users,
                 * and return an error instead of taking fallback path.

net/xfrm/xfrm_state.c +9 -13

index ad2202fa82f3..568fe8df7741 100644
@@ -424,11 +424,12 @@ void xfrm_unregister_type_offload(const struct xfrm_type_offload *type,
 }
 EXPORT_SYMBOL(xfrm_unregister_type_offload);
 
-static const struct xfrm_type_offload *
-xfrm_get_type_offload(u8 proto, unsigned short family, bool try_load)
+const struct xfrm_type_offload *xfrm_get_type_offload(u8 proto,
+                                                     unsigned short family)
 {
        const struct xfrm_type_offload *type = NULL;
        struct xfrm_state_afinfo *afinfo;
+       bool try_load = true;
 
 retry:
        afinfo = xfrm_state_get_afinfo(family);

@@ -456,11 +457,7 @@ xfrm_get_type_offload(u8 proto, unsigned short family, bool try_load)
 
        return type;
 }
-
-static void xfrm_put_type_offload(const struct xfrm_type_offload *type)
-{
-       module_put(type->owner);
-}
+EXPORT_SYMBOL(xfrm_get_type_offload);
 
 static const struct xfrm_mode xfrm4_mode_map[XFRM_MODE_MAX] = {
        [XFRM_MODE_BEET] = {

@@ -609,8 +606,6 @@ static void ___xfrm_state_destroy(struct xfrm_state *x)
        kfree(x->coaddr);
        kfree(x->replay_esn);
        kfree(x->preplay_esn);
-       if (x->type_offload)
-               xfrm_put_type_offload(x->type_offload);
        if (x->type) {
                x->type->destructor(x);
                xfrm_put_type(x->type);

@@ -784,6 +779,9 @@ void xfrm_dev_state_free(struct xfrm_state *x)
        struct xfrm_dev_offload *xso = &x->xso;
        struct net_device *dev = READ_ONCE(xso->dev);
 
+       xfrm_put_type_offload(x->type_offload);
+       x->type_offload = NULL;
+
        if (dev && dev->xfrmdev_ops) {
                spin_lock_bh(&xfrm_state_dev_gc_lock);
                if (!hlist_unhashed(&x->dev_gclist))

@@ -3122,7 +3120,7 @@ u32 xfrm_state_mtu(struct xfrm_state *x, int mtu)
 }
 EXPORT_SYMBOL_GPL(xfrm_state_mtu);
 
-int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload,
+int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
                      struct netlink_ext_ack *extack)
 {
        const struct xfrm_mode *inner_mode;

@@ -3178,8 +3176,6 @@ int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload,
                goto error;
        }
 
-       x->type_offload = xfrm_get_type_offload(x->id.proto, family, offload);
-
        err = x->type->init_state(x, extack);
        if (err)
                goto error;

@@ -3229,7 +3225,7 @@ int xfrm_init_state(struct xfrm_state *x)
 {
        int err;
 
-       err = __xfrm_init_state(x, true, false, NULL);
+       err = __xfrm_init_state(x, true, NULL);
        if (!err)
                x->km.state = XFRM_STATE_VALID;
 

net/xfrm/xfrm_user.c +1 -2

index 08c6d6f0179f..82a768500999 100644
@@ -907,7 +907,7 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
                        goto error;
        }
 
-       err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV], extack);
+       err = __xfrm_init_state(x, false, extack);
        if (err)
                goto error;
 
--

2.48.1

Hi Leon,

Quoted 15 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

XFRM offload path is probed even if offload isn't needed at all. Let's make sure that x->type_offload pointer stays NULL for such path to reduce ambiguity.

Fixes 9d389d7f84bb ("xfrm: Add a xfrm type offload.")

Signed-off-by Leon Romanovsky <[email protected]>

---
 include/net/xfrm.h     | 12 +++++++++++-
 net/xfrm/xfrm_device.c | 14 +++++++++-----
 net/xfrm/xfrm_state.c  | 22 +++++++++-------------
 net/xfrm/xfrm_user.c   |  2 +-
 4 files changed, 30 insertions(+), 20 deletions(-)

Quoted 2 lines

net/xfrm/xfrm_device.c +2 -0

+       x->type_offload = xfrm_get_type_offload(x->id.proto, x->props.family);
+       if (!x->type_offload) {

Review

Quoted 2 lines

(quoted diff) +2 -0

+               xfrm_put_type_offload(x->type_offload);
+               x->type_offload = NULL;

We always set type_offload to NULL. Can we move type_offload = NULL in xfrm_put_type_offload() ?

Thanks
-Bharat

Quoted 98 lines

/* User explicitly requested packet offload mode and configured
* policy in addition to the XFRM state. So be civil to users,
* and return an error instead of taking fallback path.

net/xfrm/xfrm_state.c +9 -13

index ad2202fa82f3..568fe8df7741 100644
@@ -424,11 +424,12 @@ void xfrm_unregister_type_offload(const struct xfrm_type_offload *type,
 }
 EXPORT_SYMBOL(xfrm_unregister_type_offload);
 
-static const struct xfrm_type_offload *
-xfrm_get_type_offload(u8 proto, unsigned short family, bool try_load)
+const struct xfrm_type_offload *xfrm_get_type_offload(u8 proto,
+                                                     unsigned short family)
 {
        const struct xfrm_type_offload *type = NULL;
        struct xfrm_state_afinfo *afinfo;
+       bool try_load = true;
 
 retry:
        afinfo = xfrm_state_get_afinfo(family);

@@ -456,11 +457,7 @@ xfrm_get_type_offload(u8 proto, unsigned short family, bool try_load)
 
        return type;
 }
-
-static void xfrm_put_type_offload(const struct xfrm_type_offload *type)
-{
-       module_put(type->owner);
-}
+EXPORT_SYMBOL(xfrm_get_type_offload);
 
 static const struct xfrm_mode xfrm4_mode_map[XFRM_MODE_MAX] = {
        [XFRM_MODE_BEET] = {

@@ -609,8 +606,6 @@ static void ___xfrm_state_destroy(struct xfrm_state *x)
        kfree(x->coaddr);
        kfree(x->replay_esn);
        kfree(x->preplay_esn);
-       if (x->type_offload)
-               xfrm_put_type_offload(x->type_offload);
        if (x->type) {
                x->type->destructor(x);
                xfrm_put_type(x->type);

@@ -784,6 +779,9 @@ void xfrm_dev_state_free(struct xfrm_state *x)
        struct xfrm_dev_offload *xso = &x->xso;
        struct net_device *dev = READ_ONCE(xso->dev);
 
+       xfrm_put_type_offload(x->type_offload);
+       x->type_offload = NULL;
+
        if (dev && dev->xfrmdev_ops) {
                spin_lock_bh(&xfrm_state_dev_gc_lock);
                if (!hlist_unhashed(&x->dev_gclist))

@@ -3122,7 +3120,7 @@ u32 xfrm_state_mtu(struct xfrm_state *x, int mtu)
 }
 EXPORT_SYMBOL_GPL(xfrm_state_mtu);
 
-int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload,
+int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
                      struct netlink_ext_ack *extack)
 {
        const struct xfrm_mode *inner_mode;

@@ -3178,8 +3176,6 @@ int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload,
                goto error;
        }
 
-       x->type_offload = xfrm_get_type_offload(x->id.proto, family, offload);
-
        err = x->type->init_state(x, extack);
        if (err)
                goto error;

@@ -3229,7 +3225,7 @@ int xfrm_init_state(struct xfrm_state *x)
 {
        int err;
 
-       err = __xfrm_init_state(x, true, false, NULL);
+       err = __xfrm_init_state(x, true, NULL);
        if (!err)
                x->km.state = XFRM_STATE_VALID;
 

net/xfrm/xfrm_user.c +1 -2

index 08c6d6f0179f..82a768500999 100644
@@ -907,7 +907,7 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
                        goto error;
        }
 
-       err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV], extack);
+       err = __xfrm_init_state(x, false, extack);
        if (err)
                goto error;
 
--

2.48.1

Quoted 18 lines · Bharat Bhushan

Hi Leon,

Quoted 15 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

XFRM offload path is probed even if offload isn't needed at all. Let's make sure that x->type_offload pointer stays NULL for such path to reduce ambiguity.

Fixes 9d389d7f84bb ("xfrm: Add a xfrm type offload.")

Signed-off-by Leon Romanovsky <[email protected]>

---
 include/net/xfrm.h     | 12 +++++++++++-
 net/xfrm/xfrm_device.c | 14 +++++++++-----
 net/xfrm/xfrm_state.c  | 22 +++++++++-------------
 net/xfrm/xfrm_user.c   |  2 +-
 4 files changed, 30 insertions(+), 20 deletions(-)

<...>

Quoted 2 lines

Quoted 2 lines

net/xfrm/xfrm_device.c +2 -0

+       x->type_offload = xfrm_get_type_offload(x->id.proto, x->props.family);
+       if (!x->type_offload) {

<...>

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+               xfrm_put_type_offload(x->type_offload);
+               x->type_offload = NULL;

We always set type_offload to NULL. Can we move type_offload = NULL in xfrm_put_type_offload() ?

We can, but it will require change to xfrm_get_type_offload() too, otherwise we will get asymmetrical get/put.

Do you want something like that?
int xfrm_get_type_offload(struct xfrm_state *x);
void xfrm_put_type_offload(struct xfrm_state *x);

Thansk

Quoted 103 lines

Thanks
-Bharat

Quoted 98 lines

/* User explicitly requested packet offload mode and configured
* policy in addition to the XFRM state. So be civil to users,
* and return an error instead of taking fallback path.

net/xfrm/xfrm_state.c +9 -13

index ad2202fa82f3..568fe8df7741 100644
@@ -424,11 +424,12 @@ void xfrm_unregister_type_offload(const struct xfrm_type_offload *type,
 }
 EXPORT_SYMBOL(xfrm_unregister_type_offload);
 
-static const struct xfrm_type_offload *
-xfrm_get_type_offload(u8 proto, unsigned short family, bool try_load)
+const struct xfrm_type_offload *xfrm_get_type_offload(u8 proto,
+                                                     unsigned short family)
 {
        const struct xfrm_type_offload *type = NULL;
        struct xfrm_state_afinfo *afinfo;
+       bool try_load = true;
 
 retry:
        afinfo = xfrm_state_get_afinfo(family);

@@ -456,11 +457,7 @@ xfrm_get_type_offload(u8 proto, unsigned short family, bool try_load)
 
        return type;
 }
-
-static void xfrm_put_type_offload(const struct xfrm_type_offload *type)
-{
-       module_put(type->owner);
-}
+EXPORT_SYMBOL(xfrm_get_type_offload);
 
 static const struct xfrm_mode xfrm4_mode_map[XFRM_MODE_MAX] = {
        [XFRM_MODE_BEET] = {

@@ -609,8 +606,6 @@ static void ___xfrm_state_destroy(struct xfrm_state *x)
        kfree(x->coaddr);
        kfree(x->replay_esn);
        kfree(x->preplay_esn);
-       if (x->type_offload)
-               xfrm_put_type_offload(x->type_offload);
        if (x->type) {
                x->type->destructor(x);
                xfrm_put_type(x->type);

@@ -784,6 +779,9 @@ void xfrm_dev_state_free(struct xfrm_state *x)
        struct xfrm_dev_offload *xso = &x->xso;
        struct net_device *dev = READ_ONCE(xso->dev);
 
+       xfrm_put_type_offload(x->type_offload);
+       x->type_offload = NULL;
+
        if (dev && dev->xfrmdev_ops) {
                spin_lock_bh(&xfrm_state_dev_gc_lock);
                if (!hlist_unhashed(&x->dev_gclist))

@@ -3122,7 +3120,7 @@ u32 xfrm_state_mtu(struct xfrm_state *x, int mtu)
 }
 EXPORT_SYMBOL_GPL(xfrm_state_mtu);
 
-int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload,
+int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
                      struct netlink_ext_ack *extack)
 {
        const struct xfrm_mode *inner_mode;

@@ -3178,8 +3176,6 @@ int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload,
                goto error;
        }
 
-       x->type_offload = xfrm_get_type_offload(x->id.proto, family, offload);
-
        err = x->type->init_state(x, extack);
        if (err)
                goto error;

@@ -3229,7 +3225,7 @@ int xfrm_init_state(struct xfrm_state *x)
 {
        int err;
 
-       err = __xfrm_init_state(x, true, false, NULL);
+       err = __xfrm_init_state(x, true, NULL);
        if (!err)
                x->km.state = XFRM_STATE_VALID;
 

net/xfrm/xfrm_user.c +1 -2

index 08c6d6f0179f..82a768500999 100644
@@ -907,7 +907,7 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
                        goto error;
        }
 
-       err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV], extack);
+       err = __xfrm_init_state(x, false, extack);
        if (err)
                goto error;
 
--

2.48.1

Quoted 28 lines · Leon Romanovsky

Quoted 18 lines · Bharat Bhushan

Hi Leon,

Quoted 15 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

XFRM offload path is probed even if offload isn't needed at all. Let's make sure that x->type_offload pointer stays NULL for such path to reduce ambiguity.

Fixes 9d389d7f84bb ("xfrm: Add a xfrm type offload.")

Signed-off-by Leon Romanovsky <[email protected]>

---
 include/net/xfrm.h     | 12 +++++++++++-
 net/xfrm/xfrm_device.c | 14 +++++++++-----
 net/xfrm/xfrm_state.c  | 22 +++++++++-------------
 net/xfrm/xfrm_user.c   |  2 +-
 4 files changed, 30 insertions(+), 20 deletions(-)

<...>

Quoted 2 lines

Quoted 2 lines

net/xfrm/xfrm_device.c +2 -0

+       x->type_offload = xfrm_get_type_offload(x->id.proto, x->props.family);
+       if (!x->type_offload) {

<...>

Quoted 8 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+               xfrm_put_type_offload(x->type_offload);
+               x->type_offload = NULL;

We always set type_offload to NULL. Can we move type_offload = NULL in xfrm_put_type_offload() ?

We can, but it will require change to xfrm_get_type_offload() too, otherwise we will get asymmetrical get/put.

"x->type_offload = NULL" is always set after the put() function. so I thought that maybe moving "x->type_offload = NULL" to the put() function would simplify.
Yes, get/put will be asymmetric. Maybe setting "x->type_offload" can be done in get/put().
Anyway it is not a major comment. ignore if this does not simplify.

Thanks
-Bharat

Quoted 110 lines

Do you want something like that?
int xfrm_get_type_offload(struct xfrm_state *x);
void xfrm_put_type_offload(struct xfrm_state *x);

Thansk

Quoted 103 lines

Thanks
-Bharat

Quoted 98 lines

/* User explicitly requested packet offload mode and configured
* policy in addition to the XFRM state. So be civil to users,
* and return an error instead of taking fallback path.

net/xfrm/xfrm_state.c +9 -13

index ad2202fa82f3..568fe8df7741 100644
@@ -424,11 +424,12 @@ void xfrm_unregister_type_offload(const struct xfrm_type_offload *type,
 }
 EXPORT_SYMBOL(xfrm_unregister_type_offload);
 
-static const struct xfrm_type_offload *
-xfrm_get_type_offload(u8 proto, unsigned short family, bool try_load)
+const struct xfrm_type_offload *xfrm_get_type_offload(u8 proto,
+                                                     unsigned short family)
 {
        const struct xfrm_type_offload *type = NULL;
        struct xfrm_state_afinfo *afinfo;
+       bool try_load = true;
 
 retry:
        afinfo = xfrm_state_get_afinfo(family);

@@ -456,11 +457,7 @@ xfrm_get_type_offload(u8 proto, unsigned short family, bool try_load)
 
        return type;
 }
-
-static void xfrm_put_type_offload(const struct xfrm_type_offload *type)
-{
-       module_put(type->owner);
-}
+EXPORT_SYMBOL(xfrm_get_type_offload);
 
 static const struct xfrm_mode xfrm4_mode_map[XFRM_MODE_MAX] = {
        [XFRM_MODE_BEET] = {

@@ -609,8 +606,6 @@ static void ___xfrm_state_destroy(struct xfrm_state *x)
        kfree(x->coaddr);
        kfree(x->replay_esn);
        kfree(x->preplay_esn);
-       if (x->type_offload)
-               xfrm_put_type_offload(x->type_offload);
        if (x->type) {
                x->type->destructor(x);
                xfrm_put_type(x->type);

@@ -784,6 +779,9 @@ void xfrm_dev_state_free(struct xfrm_state *x)
        struct xfrm_dev_offload *xso = &x->xso;
        struct net_device *dev = READ_ONCE(xso->dev);
 
+       xfrm_put_type_offload(x->type_offload);
+       x->type_offload = NULL;
+
        if (dev && dev->xfrmdev_ops) {
                spin_lock_bh(&xfrm_state_dev_gc_lock);
                if (!hlist_unhashed(&x->dev_gclist))

@@ -3122,7 +3120,7 @@ u32 xfrm_state_mtu(struct xfrm_state *x, int mtu)
 }
 EXPORT_SYMBOL_GPL(xfrm_state_mtu);
 
-int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload,
+int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
                      struct netlink_ext_ack *extack)
 {
        const struct xfrm_mode *inner_mode;

@@ -3178,8 +3176,6 @@ int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload,
                goto error;
        }
 
-       x->type_offload = xfrm_get_type_offload(x->id.proto, family, offload);
-
        err = x->type->init_state(x, extack);
        if (err)
                goto error;

@@ -3229,7 +3225,7 @@ int xfrm_init_state(struct xfrm_state *x)
 {
        int err;
 
-       err = __xfrm_init_state(x, true, false, NULL);
+       err = __xfrm_init_state(x, true, NULL);
        if (!err)
                x->km.state = XFRM_STATE_VALID;
 

net/xfrm/xfrm_user.c +1 -2

index 08c6d6f0179f..82a768500999 100644
@@ -907,7 +907,7 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
                        goto error;
        }
 
-       err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV], extack);
+       err = __xfrm_init_state(x, false, extack);
        if (err)
                goto error;
 
--

2.48.1

From: Leon Romanovsky <[email protected]>

SA replay mode is initialized differently for user-space and kernel-space users, but the call to xfrm_init_replay() existed in common path with boolean protection. That caused to situation where we have two different function orders.

So let's rewrite the SA initialization flow to have same order for both in-kernel and user-space callers.

Signed-off-by Leon Romanovsky <[email protected]>

---
 include/net/xfrm.h    |  3 +--
 net/xfrm/xfrm_state.c | 22 ++++++++++------------
 net/xfrm/xfrm_user.c  |  2 +-
 3 files changed, 12 insertions(+), 15 deletions(-)

include/net/xfrm.h +1 -2

index 28355a5be5b9..58f8f7661ec4 100644
@@ -1770,8 +1770,7 @@ void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
 u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
 int xfrm_init_replay(struct xfrm_state *x, struct netlink_ext_ack *extack);
 u32 xfrm_state_mtu(struct xfrm_state *x, int mtu);
-int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
-		      struct netlink_ext_ack *extack);
+int __xfrm_init_state(struct xfrm_state *x, struct netlink_ext_ack *extack);
 int xfrm_init_state(struct xfrm_state *x);
 int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type);
 int xfrm_input_resume(struct sk_buff *skb, int nexthdr);

net/xfrm/xfrm_state.c +0 -0

index 568fe8df7741..42799b0946a3 100644

Quoted 36 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

SA replay mode is initialized differently for user-space and kernel-space users, but the call to xfrm_init_replay() existed in common path with boolean protection. That caused to situation where we have two different function orders.

So let's rewrite the SA initialization flow to have same order for both in-kernel and user-space callers.

Signed-off-by Leon Romanovsky <[email protected]>

---
 include/net/xfrm.h    |  3 +--
 net/xfrm/xfrm_state.c | 22 ++++++++++------------
 net/xfrm/xfrm_user.c  |  2 +-
 3 files changed, 12 insertions(+), 15 deletions(-)

include/net/xfrm.h +1 -2

index 28355a5be5b9..58f8f7661ec4 100644
@@ -1770,8 +1770,7 @@ void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
 u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
 int xfrm_init_replay(struct xfrm_state *x, struct netlink_ext_ack *extack);
 u32 xfrm_state_mtu(struct xfrm_state *x, int mtu);
-int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
-		      struct netlink_ext_ack *extack);
+int __xfrm_init_state(struct xfrm_state *x, struct netlink_ext_ack *extack);
 int xfrm_init_state(struct xfrm_state *x);
 int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type);
 int xfrm_input_resume(struct sk_buff *skb, int nexthdr);

net/xfrm/xfrm_state.c +0 -0

index 568fe8df7741..42799b0946a3 100644

net/xfrm/xfrm_state.c u32 xfrm_state_mtu(struct xfrm_state *x, int mtu) · line 3123

Quoted 6 lines

net/xfrm/xfrm_state.c +1 -2

@@ -3120,8 +3120,7 @@ u32 xfrm_state_mtu(struct xfrm_state *x, int mtu)
 }
 EXPORT_SYMBOL_GPL(xfrm_state_mtu);
 
-int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
-		      struct netlink_ext_ack *extack)
+int __xfrm_init_state(struct xfrm_state *x, struct netlink_ext_ack *extack)

The whole point of having __xfrm_init_state was to sepatate codepaths that need init_replay and those who don't need it. That was a bandaid for something, unfortunately I don't remenber for what.

If we don't need that anymore, maybe we can merge __xfrm_init_state into xfrm_init_state, as it was before.

The rest of the patchset looks OK to me.

Quoted 49 lines · Steffen Klassert

Quoted 36 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

SA replay mode is initialized differently for user-space and kernel-space users, but the call to xfrm_init_replay() existed in common path with boolean protection. That caused to situation where we have two different function orders.

So let's rewrite the SA initialization flow to have same order for both in-kernel and user-space callers.

Signed-off-by Leon Romanovsky <[email protected]>

---
 include/net/xfrm.h    |  3 +--
 net/xfrm/xfrm_state.c | 22 ++++++++++------------
 net/xfrm/xfrm_user.c  |  2 +-
 3 files changed, 12 insertions(+), 15 deletions(-)

include/net/xfrm.h +1 -2

index 28355a5be5b9..58f8f7661ec4 100644
@@ -1770,8 +1770,7 @@ void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
 u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
 int xfrm_init_replay(struct xfrm_state *x, struct netlink_ext_ack *extack);
 u32 xfrm_state_mtu(struct xfrm_state *x, int mtu);
-int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
-		      struct netlink_ext_ack *extack);
+int __xfrm_init_state(struct xfrm_state *x, struct netlink_ext_ack *extack);
 int xfrm_init_state(struct xfrm_state *x);
 int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type);
 int xfrm_input_resume(struct sk_buff *skb, int nexthdr);

net/xfrm/xfrm_state.c +0 -0

index 568fe8df7741..42799b0946a3 100644

net/xfrm/xfrm_state.c u32 xfrm_state_mtu(struct xfrm_state *x, int mtu) · line 3123

Quoted 6 lines

net/xfrm/xfrm_state.c +1 -2

@@ -3120,8 +3120,7 @@ u32 xfrm_state_mtu(struct xfrm_state *x, int mtu)
 }
 EXPORT_SYMBOL_GPL(xfrm_state_mtu);
 
-int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
-		      struct netlink_ext_ack *extack)
+int __xfrm_init_state(struct xfrm_state *x, struct netlink_ext_ack *extack)

The whole point of having __xfrm_init_state was to sepatate codepaths that need init_replay and those who don't need it. That was a bandaid for something, unfortunately I don't remenber for what.

Quoted 3 lines

If we don't need that anymore, maybe we can merge __xfrm_init_state into xfrm_init_state, as it was before.

Main difference between __xfrm_init_state and xfrm_init_state is that latter is called without extack, which doesn't exist in kernel path.

Quoted 54 lines · Leon Romanovsky

Quoted 52 lines · Steffen Klassert

Quoted 36 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

SA replay mode is initialized differently for user-space and kernel-space users, but the call to xfrm_init_replay() existed in common path with boolean protection. That caused to situation where we have two different function orders.

So let's rewrite the SA initialization flow to have same order for both in-kernel and user-space callers.

Signed-off-by Leon Romanovsky <[email protected]>

---
 include/net/xfrm.h    |  3 +--
 net/xfrm/xfrm_state.c | 22 ++++++++++------------
 net/xfrm/xfrm_user.c  |  2 +-
 3 files changed, 12 insertions(+), 15 deletions(-)

include/net/xfrm.h +1 -2

index 28355a5be5b9..58f8f7661ec4 100644
@@ -1770,8 +1770,7 @@ void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
 u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
 int xfrm_init_replay(struct xfrm_state *x, struct netlink_ext_ack *extack);
 u32 xfrm_state_mtu(struct xfrm_state *x, int mtu);
-int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
-		      struct netlink_ext_ack *extack);
+int __xfrm_init_state(struct xfrm_state *x, struct netlink_ext_ack *extack);
 int xfrm_init_state(struct xfrm_state *x);
 int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type);
 int xfrm_input_resume(struct sk_buff *skb, int nexthdr);

net/xfrm/xfrm_state.c +0 -0

index 568fe8df7741..42799b0946a3 100644

net/xfrm/xfrm_state.c u32 xfrm_state_mtu(struct xfrm_state *x, int mtu) · line 3123

Quoted 6 lines

net/xfrm/xfrm_state.c +1 -2

@@ -3120,8 +3120,7 @@ u32 xfrm_state_mtu(struct xfrm_state *x, int mtu)
 }
 EXPORT_SYMBOL_GPL(xfrm_state_mtu);
 
-int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
-		      struct netlink_ext_ack *extack)
+int __xfrm_init_state(struct xfrm_state *x, struct netlink_ext_ack *extack)

The whole point of having __xfrm_init_state was to sepatate codepaths that need init_replay and those who don't need it. That was a bandaid for something, unfortunately I don't remenber for what.

If we don't need that anymore, maybe we can merge __xfrm_init_state into xfrm_init_state, as it was before.

Quoted 2 lines

Main difference between __xfrm_init_state and xfrm_init_state is that latter is called without extack, which doesn't exist in kernel path.

That split happened ~ 15 years ago, we did not have extack back than. But I'm also ok with keeping it if extack is a reason for it.

Do you plan to respin, or should I take the patchset as is?

Thanks!

From: Leon Romanovsky <[email protected]>

Almost all drivers except bond and nsim had same check if device can perform XFRM offload on that specific packet. The check was that packet doesn't have IPv4 options and IPv6 extensions.

In NIC drivers, the IPv4 HELEN comparison was slightly different, but the intent was to check for the same conditions. So let's chose more strict variant as a common base.

Signed-off-by Leon Romanovsky <[email protected]>

---
  Documentation/networking/xfrm_device.rst      |  3 ++-
  drivers/net/bonding/bond_main.c               | 16 +++++---------
  .../net/ethernet/chelsio/cxgb4/cxgb4_main.c   | 21 -------------------
  .../inline_crypto/ch_ipsec/chcr_ipsec.c       | 16 --------------
  .../net/ethernet/intel/ixgbe/ixgbe_ipsec.c    | 21 -------------------
  drivers/net/ethernet/intel/ixgbevf/ipsec.c    | 21 -------------------
  .../marvell/octeontx2/nic/cn10k_ipsec.c       | 15 -------------
  .../mellanox/mlx5/core/en_accel/ipsec.c       | 16 --------------
  .../net/ethernet/netronome/nfp/crypto/ipsec.c | 11 ----------
  drivers/net/netdevsim/ipsec.c                 | 11 ----------
  drivers/net/netdevsim/netdevsim.h             |  1 -
  net/xfrm/xfrm_device.c                        | 15 +++++++++++++
  12 files changed, 22 insertions(+), 145 deletions(-)

Documentation/networking/xfrm_device.rst +0 -0

index 66f6e9a9b59a..39bb98939d1f 100644

Documentation/networking/xfrm_device.rst +1 -0

+this callback, the stack provides reasonable defaults.
 
  Crypto offload mode:
  When ready to send, the driver needs to inspect the Tx packet for the

drivers/net/bonding/bond_main.c +0 -0

index e45bba240cbc..bfb55c23380b 100644
@@ -676,22 +676,16 @@ static void bond_ipsec_free_sa(struct xfrm_state *xs)
  static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)

Quoted 31 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Almost all drivers except bond and nsim had same check if device can perform XFRM offload on that specific packet. The check was that packet doesn't have IPv4 options and IPv6 extensions.

In NIC drivers, the IPv4 HELEN comparison was slightly different, but the intent was to check for the same conditions. So let's chose more strict variant as a common base.

Signed-off-by Leon Romanovsky <[email protected]>

---
  Documentation/networking/xfrm_device.rst      |  3 ++-
  drivers/net/bonding/bond_main.c               | 16 +++++---------
  .../net/ethernet/chelsio/cxgb4/cxgb4_main.c   | 21 -------------------
  .../inline_crypto/ch_ipsec/chcr_ipsec.c       | 16 --------------
  .../net/ethernet/intel/ixgbe/ixgbe_ipsec.c    | 21 -------------------
  drivers/net/ethernet/intel/ixgbevf/ipsec.c    | 21 -------------------
  .../marvell/octeontx2/nic/cn10k_ipsec.c       | 15 -------------
  .../mellanox/mlx5/core/en_accel/ipsec.c       | 16 --------------
  .../net/ethernet/netronome/nfp/crypto/ipsec.c | 11 ----------
  drivers/net/netdevsim/ipsec.c                 | 11 ----------
  drivers/net/netdevsim/netdevsim.h             |  1 -
  net/xfrm/xfrm_device.c                        | 15 +++++++++++++
  12 files changed, 22 insertions(+), 145 deletions(-)

Documentation/networking/xfrm_device.rst +0 -0

index 66f6e9a9b59a..39bb98939d1f 100644

Documentation/networking/xfrm_device.rst been setup for offload, it first calls into xdo_dev_offload_ok() with · line 129

Quoted 5 lines

Documentation/networking/xfrm_device.rst +1 -1

@@ -126,7 +126,8 @@ been setup for offload, it first calls into xdo_dev_offload_ok() with
  the skb and the intended offload state to ask the driver if the offload
  will serviceable.  This can check the packet information to be sure the
  offload can be supported (e.g. IPv4 or IPv6, no IPv4 options, etc) and
-return true of false to signify its support.
+return true of false to signify its support. In case driver doesn't implement

In this commit, remove the functions cxgb4_ipsec_offload_ok, ch_ipsec_offload_ok, ixgbe_ipsec_offload_ok, ixgbevf_ipsec_offload_ok, cn10k_ipsec_offload_ok, mlx5e_ipsec_offload_ok, nfp_net_ipsec_offload_ok and nsim_ipsec_offload_ok, use the function xfrm_dev_offload_ok to do the same work.

But in the file xfrm_device.rst, "return true or false to signify its support"?

of --> should be "or"

Thanks a lot.

Reviewed-by Zhu Yanjun <[email protected]>

Zhu Yanjun

Quoted 9 lines

Documentation/networking/xfrm_device.rst +1 -0

+this callback, the stack provides reasonable defaults.
  Crypto offload mode:
  When ready to send, the driver needs to inspect the Tx packet for the

drivers/net/bonding/bond_main.c +0 -0

index e45bba240cbc..bfb55c23380b 100644
@@ -676,22 +676,16 @@ static void bond_ipsec_free_sa(struct xfrm_state *xs)
  static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)

Quoted 43 lines · Zhu Yanjun

Quoted 31 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Almost all drivers except bond and nsim had same check if device can perform XFRM offload on that specific packet. The check was that packet doesn't have IPv4 options and IPv6 extensions.

In NIC drivers, the IPv4 HELEN comparison was slightly different, but the intent was to check for the same conditions. So let's chose more strict variant as a common base.

Signed-off-by Leon Romanovsky <[email protected]>

---
   Documentation/networking/xfrm_device.rst      |  3 ++-
   drivers/net/bonding/bond_main.c               | 16 +++++---------
   .../net/ethernet/chelsio/cxgb4/cxgb4_main.c   | 21 -------------------
   .../inline_crypto/ch_ipsec/chcr_ipsec.c       | 16 --------------
   .../net/ethernet/intel/ixgbe/ixgbe_ipsec.c    | 21 -------------------
   drivers/net/ethernet/intel/ixgbevf/ipsec.c    | 21 -------------------
   .../marvell/octeontx2/nic/cn10k_ipsec.c       | 15 -------------
   .../mellanox/mlx5/core/en_accel/ipsec.c       | 16 --------------
   .../net/ethernet/netronome/nfp/crypto/ipsec.c | 11 ----------
   drivers/net/netdevsim/ipsec.c                 | 11 ----------
   drivers/net/netdevsim/netdevsim.h             |  1 -
   net/xfrm/xfrm_device.c                        | 15 +++++++++++++
   12 files changed, 22 insertions(+), 145 deletions(-)

Documentation/networking/xfrm_device.rst +0 -0

index 66f6e9a9b59a..39bb98939d1f 100644

Documentation/networking/xfrm_device.rst been setup for offload, it first calls into xdo_dev_offload_ok() with · line 129

Quoted 5 lines

Documentation/networking/xfrm_device.rst +1 -1

@@ -126,7 +126,8 @@ been setup for offload, it first calls into xdo_dev_offload_ok() with
   the skb and the intended offload state to ask the driver if the offload
   will serviceable.  This can check the packet information to be sure the
   offload can be supported (e.g. IPv4 or IPv6, no IPv4 options, etc) and
-return true of false to signify its support.
+return true of false to signify its support. In case driver doesn't implement

In this commit, remove the functions cxgb4_ipsec_offload_ok, ch_ipsec_offload_ok, ixgbe_ipsec_offload_ok, ixgbevf_ipsec_offload_ok, cn10k_ipsec_offload_ok, mlx5e_ipsec_offload_ok, nfp_net_ipsec_offload_ok and nsim_ipsec_offload_ok, use the function xfrm_dev_offload_ok to do the same work.

Quoted 2 lines

But in the file xfrm_device.rst, "return true or false to signify its support"?

This sentence continued in the xfrm_device.rst: "... In case driver doesn't implement this callback, the stack provides reasonable defaults."

Quoted 5 lines

of --> should be "or"

Thanks a lot.

Reviewed-by Zhu Yanjun <[email protected]>

Thanks

Quoted 11 lines

Zhu Yanjun

Quoted 9 lines

Documentation/networking/xfrm_device.rst +1 -0

+this callback, the stack provides reasonable defaults.
   Crypto offload mode:
   When ready to send, the driver needs to inspect the Tx packet for the

drivers/net/bonding/bond_main.c +0 -0

index e45bba240cbc..bfb55c23380b 100644
@@ -676,22 +676,16 @@ static void bond_ipsec_free_sa(struct xfrm_state *xs)
   static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)

From: Leon Romanovsky <[email protected]>

After change of initialization of x->type_offload pointer to be valid only for offloaded SAs. There is no need to rely both on x->type_offload

and x->xso.type to determine if SA is offloaded or not.

Signed-off-by Leon Romanovsky <[email protected]>