[PATCH net-next 0/4] net/tls: small code cleanup

[PATCH net-next 0/4] net/tls: small code cleanup

[Jakub Kicinski]

Hi!

This small patch set cleans up tls (mostly offload parts).
Other than avoiding unnecessary error messages - no functional
changes here.

Jakub Kicinski (4):
  net/tls: don't log errors every time offload can't proceed
  net/tls: remove old exports of sk_destruct functions
  net/tls: move definition of tls ops into net/tls.h
  net/tls: byte swap device req TCP seq no upon setting

 include/linux/netdevice.h | 23 +-------------------
 include/net/tls.h         | 21 +++++++++++++++---
 net/tls/tls_device.c      | 45 +++++++++++++++++++--------------------
 3 files changed, 41 insertions(+), 48 deletions(-)

-- 
2.21.0

[PATCH net-next 1/4] net/tls: don't log errors every time offload can't proceed

[Jakub Kicinski]

Currently when CONFIG_TLS_DEVICE is set each time kTLS
connection is opened and the offload is not successful
(either because the underlying device doesn't support
it or e.g. it's tables are full) a rate limited error
will be printed to the logs.

There is nothing wrong with failing TLS offload.  SW
path will process the packets just fine, avoid the
noisy messages by demoting them from error to debug
level.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 net/tls/tls_device.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index 9f3bdbc1e593..9d67989ab7a7 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -865,8 +865,8 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
 	}
 
 	if (!(netdev->features & NETIF_F_HW_TLS_RX)) {
-		pr_err_ratelimited("%s: netdev %s with no TLS offload\n",
-				   __func__, netdev->name);
+		pr_debug_ratelimited("%s: netdev %s with no TLS offload\n",
+				     __func__, netdev->name);
 		rc = -ENOTSUPP;
 		goto release_netdev;
 	}
@@ -895,8 +895,8 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
 					     &ctx->crypto_recv.info,
 					     tcp_sk(sk)->copied_seq);
 	if (rc) {
-		pr_err_ratelimited("%s: The netdev has refused to offload this socket\n",
-				   __func__);
+		pr_debug_ratelimited("%s: The netdev has refused to offload this socket\n",
+				     __func__);
 		goto free_sw_resources;
 	}
 
-- 
2.21.0

[PATCH net-next 2/4] net/tls: remove old exports of sk_destruct functions

[Jakub Kicinski]

tls_device_sk_destruct being set on a socket used to indicate
that socket is a kTLS device one.  That is no longer true -
now we use sk_validate_xmit_skb pointer for that purpose.
Remove the export.  tls_device_attach() needs to be moved.

While at it, remove the dead declaration of tls_sk_destruct().

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
Reviewed-by: Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 include/net/tls.h    |  2 --
 net/tls/tls_device.c | 35 +++++++++++++++++------------------
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index d9d0ac66f040..20196cb31ecc 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -317,7 +317,6 @@ int tls_set_device_offload(struct sock *sk, struct tls_context *ctx);
 int tls_device_sendmsg(struct sock *sk, struct msghdr *msg, size_t size);
 int tls_device_sendpage(struct sock *sk, struct page *page,
 			int offset, size_t size, int flags);
-void tls_device_sk_destruct(struct sock *sk);
 void tls_device_free_resources_tx(struct sock *sk);
 void tls_device_init(void);
 void tls_device_cleanup(void);
@@ -336,7 +335,6 @@ static inline u32 tls_record_start_seq(struct tls_record_info *rec)
 	return rec->end_seq - rec->len;
 }
 
-void tls_sk_destruct(struct sock *sk, struct tls_context *ctx);
 int tls_push_sg(struct sock *sk, struct tls_context *ctx,
 		struct scatterlist *sg, u16 first_offset,
 		int flags);
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index 9d67989ab7a7..cb368efe3567 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -89,22 +89,6 @@ static void tls_device_gc_task(struct work_struct *work)
 	}
 }
 
-static void tls_device_attach(struct tls_context *ctx, struct sock *sk,
-			      struct net_device *netdev)
-{
-	if (sk->sk_destruct != tls_device_sk_destruct) {
-		refcount_set(&ctx->refcount, 1);
-		dev_hold(netdev);
-		ctx->netdev = netdev;
-		spin_lock_irq(&tls_device_lock);
-		list_add_tail(&ctx->list, &tls_device_list);
-		spin_unlock_irq(&tls_device_lock);
-
-		ctx->sk_destruct = sk->sk_destruct;
-		sk->sk_destruct = tls_device_sk_destruct;
-	}
-}
-
 static void tls_device_queue_ctx_destruction(struct tls_context *ctx)
 {
 	unsigned long flags;
@@ -199,7 +183,7 @@ static void tls_icsk_clean_acked(struct sock *sk, u32 acked_seq)
  * socket and no in-flight SKBs associated with this
  * socket, so it is safe to free all the resources.
  */
-void tls_device_sk_destruct(struct sock *sk)
+static void tls_device_sk_destruct(struct sock *sk)
 {
 	struct tls_context *tls_ctx = tls_get_ctx(sk);
 	struct tls_offload_context_tx *ctx = tls_offload_ctx_tx(tls_ctx);
@@ -217,7 +201,6 @@ void tls_device_sk_destruct(struct sock *sk)
 	if (refcount_dec_and_test(&tls_ctx->refcount))
 		tls_device_queue_ctx_destruction(tls_ctx);
 }
-EXPORT_SYMBOL(tls_device_sk_destruct);
 
 void tls_device_free_resources_tx(struct sock *sk)
 {
@@ -682,6 +665,22 @@ int tls_device_decrypted(struct sock *sk, struct sk_buff *skb)
 		tls_device_reencrypt(sk, skb);
 }
 
+static void tls_device_attach(struct tls_context *ctx, struct sock *sk,
+			      struct net_device *netdev)
+{
+	if (sk->sk_destruct != tls_device_sk_destruct) {
+		refcount_set(&ctx->refcount, 1);
+		dev_hold(netdev);
+		ctx->netdev = netdev;
+		spin_lock_irq(&tls_device_lock);
+		list_add_tail(&ctx->list, &tls_device_list);
+		spin_unlock_irq(&tls_device_lock);
+
+		ctx->sk_destruct = sk->sk_destruct;
+		sk->sk_destruct = tls_device_sk_destruct;
+	}
+}
+
 int tls_set_device_offload(struct sock *sk, struct tls_context *ctx)
 {
 	u16 nonce_size, tag_size, iv_size, rec_seq_size;
-- 
2.21.0

[PATCH net-next 3/4] net/tls: move definition of tls ops into net/tls.h

[Jakub Kicinski]

There seems to be no reason for tls_ops to be defined in netdevice.h
which is included in a lot of places.  Don't wrap the struct/enum
declaration in ifdefs, it trickles down unnecessary ifdefs into
driver code.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 include/linux/netdevice.h | 23 +----------------------
 include/net/tls.h         | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c46d218a0456..44b47e9df94a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -914,34 +914,13 @@ struct xfrmdev_ops {
 };
 #endif
 
-#if IS_ENABLED(CONFIG_TLS_DEVICE)
-enum tls_offload_ctx_dir {
-	TLS_OFFLOAD_CTX_DIR_RX,
-	TLS_OFFLOAD_CTX_DIR_TX,
-};
-
-struct tls_crypto_info;
-struct tls_context;
-
-struct tlsdev_ops {
-	int (*tls_dev_add)(struct net_device *netdev, struct sock *sk,
-			   enum tls_offload_ctx_dir direction,
-			   struct tls_crypto_info *crypto_info,
-			   u32 start_offload_tcp_sn);
-	void (*tls_dev_del)(struct net_device *netdev,
-			    struct tls_context *ctx,
-			    enum tls_offload_ctx_dir direction);
-	void (*tls_dev_resync_rx)(struct net_device *netdev,
-				  struct sock *sk, u32 seq, u64 rcd_sn);
-};
-#endif
-
 struct dev_ifalias {
 	struct rcu_head rcuhead;
 	char ifalias[];
 };
 
 struct devlink;
+struct tlsdev_ops;
 
 /*
  * This structure defines the management hooks for network devices.
diff --git a/include/net/tls.h b/include/net/tls.h
index 20196cb31ecc..41a2ee643fc5 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -277,6 +277,23 @@ struct tls_context {
 	void (*unhash)(struct sock *sk);
 };
 
+enum tls_offload_ctx_dir {
+	TLS_OFFLOAD_CTX_DIR_RX,
+	TLS_OFFLOAD_CTX_DIR_TX,
+};
+
+struct tlsdev_ops {
+	int (*tls_dev_add)(struct net_device *netdev, struct sock *sk,
+			   enum tls_offload_ctx_dir direction,
+			   struct tls_crypto_info *crypto_info,
+			   u32 start_offload_tcp_sn);
+	void (*tls_dev_del)(struct net_device *netdev,
+			    struct tls_context *ctx,
+			    enum tls_offload_ctx_dir direction);
+	void (*tls_dev_resync_rx)(struct net_device *netdev,
+				  struct sock *sk, u32 seq, u64 rcd_sn);
+};
+
 struct tls_offload_context_rx {
 	/* sw must be the first member of tls_offload_context_rx */
 	struct tls_sw_context_rx sw;
-- 
2.21.0

[PATCH net-next 4/4] net/tls: byte swap device req TCP seq no upon setting

[Jakub Kicinski]

To avoid a sparse warning byteswap the be32 sequence number
before it's stored in the atomic value.  While at it drop
unnecessary brackets and use kernel's u64 type.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 include/net/tls.h    | 2 +-
 net/tls/tls_device.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index 41a2ee643fc5..39ea62f0c1f6 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -562,7 +562,7 @@ static inline void tls_offload_rx_resync_request(struct sock *sk, __be32 seq)
 	struct tls_context *tls_ctx = tls_get_ctx(sk);
 	struct tls_offload_context_rx *rx_ctx = tls_offload_ctx_rx(tls_ctx);
 
-	atomic64_set(&rx_ctx->resync_req, ((((uint64_t)seq) << 32) | 1));
+	atomic64_set(&rx_ctx->resync_req, ((u64)ntohl(seq) << 32) | 1);
 }
 
 
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index cb368efe3567..6686013b4e9e 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -567,7 +567,7 @@ void handle_device_resync(struct sock *sk, u32 seq, u64 rcd_sn)
 
 	rx_ctx = tls_offload_ctx_rx(tls_ctx);
 	resync_req = atomic64_read(&rx_ctx->resync_req);
-	req_seq = ntohl(resync_req >> 32) - ((u32)TLS_HEADER_SIZE - 1);
+	req_seq = (resync_req >> 32) - ((u32)TLS_HEADER_SIZE - 1);
 	is_req_pending = resync_req;
 
 	if (unlikely(is_req_pending) && req_seq == seq &&
-- 
2.21.0

Re: [PATCH net-next 1/4] net/tls: don't log errors every time offload can't proceed

[Saeed Mahameed]

On Thu, 2019-04-25 at 09:56 -0700, Jakub Kicinski wrote:
> Currently when CONFIG_TLS_DEVICE is set each time kTLS
> connection is opened and the offload is not successful
> (either because the underlying device doesn't support
> it or e.g. it's tables are full) a rate limited error
> will be printed to the logs.
> 
> There is nothing wrong with failing TLS offload.  SW
> path will process the packets just fine, avoid the
> noisy messages by demoting them from error to debug
> level.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Reviewed-by: Simon Horman <simon.horman@netronome.com>

Did simon review this patch twice ;) ?
please fixup "Reviewed-by:" Tag.

and also i think it shouldn't be even debug, it should be silent.
The device didn't advertise the feature, why would we print a debug
message on every TLS connection ?


> ---
>  net/tls/tls_device.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index 9f3bdbc1e593..9d67989ab7a7 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -865,8 +865,8 @@ int tls_set_device_offload_rx(struct sock *sk,
> struct tls_context *ctx)
>  	}
>  
>  	if (!(netdev->features & NETIF_F_HW_TLS_RX)) {
> -		pr_err_ratelimited("%s: netdev %s with no TLS
> offload\n",
> -				   __func__, netdev->name);
> +		pr_debug_ratelimited("%s: netdev %s with no TLS
> offload\n",
> +				     __func__, netdev->name);
>  		rc = -ENOTSUPP;
>  		goto release_netdev;
>  	}
> @@ -895,8 +895,8 @@ int tls_set_device_offload_rx(struct sock *sk,
> struct tls_context *ctx)
>  					     &ctx->crypto_recv.info,
>  					     tcp_sk(sk)->copied_seq);
>  	if (rc) {
> -		pr_err_ratelimited("%s: The netdev has refused to
> offload this socket\n",
> -				   __func__);
> +		pr_debug_ratelimited("%s: The netdev has refused to
> offload this socket\n",
> +				     __func__);
>  		goto free_sw_resources;
>  	}
>  

Re: [PATCH net-next 1/4] net/tls: don't log errors every time offload can't proceed

[Jakub Kicinski]

On Thu, 25 Apr 2019 18:37:19 +0000, Saeed Mahameed wrote:
> On Thu, 2019-04-25 at 09:56 -0700, Jakub Kicinski wrote:
> > Currently when CONFIG_TLS_DEVICE is set each time kTLS
> > connection is opened and the offload is not successful
> > (either because the underlying device doesn't support
> > it or e.g. it's tables are full) a rate limited error
> > will be printed to the logs.
> > 
> > There is nothing wrong with failing TLS offload.  SW
> > path will process the packets just fine, avoid the
> > noisy messages by demoting them from error to debug
> > level.
> > 
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Reviewed-by: Reviewed-by: Simon Horman <simon.horman@netronome.com>  
> 
> Did simon review this patch twice ;) ?
> please fixup "Reviewed-by:" Tag.

Ups :)

> and also i think it shouldn't be even debug, it should be silent.
> The device didn't advertise the feature, why would we print a debug
> message on every TLS connection ?

Coin toss :)  I was wondering if the original author seen some value 
in those.  I have some tls tracepoints queued, those should cover set.

Re: [PATCH net-next 3/4] net/tls: move definition of tls ops into net/tls.h

[Saeed Mahameed]

On Thu, 2019-04-25 at 09:56 -0700, Jakub Kicinski wrote:
> There seems to be no reason for tls_ops to be defined in netdevice.h
> which is included in a lot of places.  Don't wrap the struct/enum
> declaration in ifdefs, it trickles down unnecessary ifdefs into
> driver code.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Reviewed-by: Simon Horman <simon.horman@netronome.com>
> 

you need to fixup the above tag in all patches

LGTM

> ---
>  include/linux/netdevice.h | 23 +----------------------
>  include/net/tls.h         | 17 +++++++++++++++++
>  2 files changed, 18 insertions(+), 22 deletions(-)
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index c46d218a0456..44b47e9df94a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -914,34 +914,13 @@ struct xfrmdev_ops {
>  };
>  #endif
>  
> -#if IS_ENABLED(CONFIG_TLS_DEVICE)
> -enum tls_offload_ctx_dir {
> -	TLS_OFFLOAD_CTX_DIR_RX,
> -	TLS_OFFLOAD_CTX_DIR_TX,
> -};
> -
> -struct tls_crypto_info;
> -struct tls_context;
> -
> -struct tlsdev_ops {
> -	int (*tls_dev_add)(struct net_device *netdev, struct sock *sk,
> -			   enum tls_offload_ctx_dir direction,
> -			   struct tls_crypto_info *crypto_info,
> -			   u32 start_offload_tcp_sn);
> -	void (*tls_dev_del)(struct net_device *netdev,
> -			    struct tls_context *ctx,
> -			    enum tls_offload_ctx_dir direction);
> -	void (*tls_dev_resync_rx)(struct net_device *netdev,
> -				  struct sock *sk, u32 seq, u64
> rcd_sn);
> -};
> -#endif
> -
>  struct dev_ifalias {
>  	struct rcu_head rcuhead;
>  	char ifalias[];
>  };
>  
>  struct devlink;
> +struct tlsdev_ops;
>  
>  /*
>   * This structure defines the management hooks for network devices.
> diff --git a/include/net/tls.h b/include/net/tls.h
> index 20196cb31ecc..41a2ee643fc5 100644
> --- a/include/net/tls.h
> +++ b/include/net/tls.h
> @@ -277,6 +277,23 @@ struct tls_context {
>  	void (*unhash)(struct sock *sk);
>  };
>  
> +enum tls_offload_ctx_dir {
> +	TLS_OFFLOAD_CTX_DIR_RX,
> +	TLS_OFFLOAD_CTX_DIR_TX,
> +};
> +
> +struct tlsdev_ops {
> +	int (*tls_dev_add)(struct net_device *netdev, struct sock *sk,
> +			   enum tls_offload_ctx_dir direction,
> +			   struct tls_crypto_info *crypto_info,
> +			   u32 start_offload_tcp_sn);
> +	void (*tls_dev_del)(struct net_device *netdev,
> +			    struct tls_context *ctx,
> +			    enum tls_offload_ctx_dir direction);
> +	void (*tls_dev_resync_rx)(struct net_device *netdev,
> +				  struct sock *sk, u32 seq, u64
> rcd_sn);
> +};
> +
>  struct tls_offload_context_rx {
>  	/* sw must be the first member of tls_offload_context_rx */
>  	struct tls_sw_context_rx sw;

Re: [PATCH net-next 4/4] net/tls: byte swap device req TCP seq no upon setting

[Saeed Mahameed]

On Thu, 2019-04-25 at 09:56 -0700, Jakub Kicinski wrote:
> To avoid a sparse warning byteswap the be32 sequence number
> before it's stored in the atomic value.  While at it drop
> unnecessary brackets and use kernel's u64 type.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Reviewed-by: Simon Horman <simon.horman@netronome.com>
> ---
>  include/net/tls.h    | 2 +-
>  net/tls/tls_device.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/tls.h b/include/net/tls.h
> index 41a2ee643fc5..39ea62f0c1f6 100644
> --- a/include/net/tls.h
> +++ b/include/net/tls.h
> @@ -562,7 +562,7 @@ static inline void
> tls_offload_rx_resync_request(struct sock *sk, __be32 seq)
>  	struct tls_context *tls_ctx = tls_get_ctx(sk);
>  	struct tls_offload_context_rx *rx_ctx =
> tls_offload_ctx_rx(tls_ctx);
>  
> -	atomic64_set(&rx_ctx->resync_req, ((((uint64_t)seq) << 32) |
> 1));
> +	atomic64_set(&rx_ctx->resync_req, ((u64)ntohl(seq) << 32) | 1);
>  }
>  
>  
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index cb368efe3567..6686013b4e9e 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -567,7 +567,7 @@ void (struct sock *sk, u32
> seq, u64 rcd_sn)
>  
>  	rx_ctx = tls_offload_ctx_rx(tls_ctx);
>  	resync_req = atomic64_read(&rx_ctx->resync_req);
> -	req_seq = ntohl(resync_req >> 32) - ((u32)TLS_HEADER_SIZE - 1);
> +	req_seq = (resync_req >> 32) - ((u32)TLS_HEADER_SIZE - 1);

this is not equivalent to what was before, 
resync_req is expected to be in network order, 
(TLS_HEADER_SIZE -1) is still in cpu indianness.


>  	is_req_pending = resync_req;
>  
>  	if (unlikely(is_req_pending) && req_seq == seq &&

Re: [PATCH net-next 4/4] net/tls: byte swap device req TCP seq no upon setting

[Jakub Kicinski]

On Thu, 25 Apr 2019 19:31:51 +0000, Saeed Mahameed wrote:
> > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> > index cb368efe3567..6686013b4e9e 100644
> > --- a/net/tls/tls_device.c
> > +++ b/net/tls/tls_device.c
> > @@ -567,7 +567,7 @@ void (struct sock *sk, u32
> > seq, u64 rcd_sn)
> >  
> >  	rx_ctx = tls_offload_ctx_rx(tls_ctx);
> >  	resync_req = atomic64_read(&rx_ctx->resync_req);
> > -	req_seq = ntohl(resync_req >> 32) - ((u32)TLS_HEADER_SIZE - 1);
> > +	req_seq = (resync_req >> 32) - ((u32)TLS_HEADER_SIZE - 1);  
> 
> this is not equivalent to what was before, 
> resync_req is expected to be in network order, 
> (TLS_HEADER_SIZE -1) is still in cpu indianness.

Naw, I think they are both in host order.

The driver passes network order.

But the stack has it in host order, this is the call site:

#ifdef CONFIG_TLS_DEVICE
	handle_device_resync(strp->sk, TCP_SKB_CB(skb)->seq + rxm->offset,
			     *(u64*)tls_ctx->rx.rec_seq);
#endif

The value passed by the driver used to be byte swapped when read from
the atomic, but I moved the byte swap to when it's stored to the atomic.
We used to have a weird situation where the atomic would have a be32 on
the top 32bits, and lower 32 bits would store the 1, in host order.

IOW the tls_offload_rx_resync_request() is the only thing setting this
value and I moved the byte swap there.

$ git grep '.->resync_req'
include/net/tls.h:      atomic64_set(&rx_ctx->resync_req, ((u64)ntohl(seq) << 32) | 1);
net/tls/tls_device.c:   resync_req = atomic64_read(&rx_ctx->resync_req);
net/tls/tls_device.c:       atomic64_try_cmpxchg(&rx_ctx->resync_req, &resync_req, 0))

Did I miss something or screw up tls_offload_rx_resync_request()?

Re: [PATCH net-next 4/4] net/tls: byte swap device req TCP seq no upon setting

[Saeed Mahameed]

On Thu, 2019-04-25 at 12:38 -0700, Jakub Kicinski wrote:
> On Thu, 25 Apr 2019 19:31:51 +0000, Saeed Mahameed wrote:
> > > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> > > index cb368efe3567..6686013b4e9e 100644
> > > --- a/net/tls/tls_device.c
> > > +++ b/net/tls/tls_device.c
> > > @@ -567,7 +567,7 @@ void (struct sock *sk, u32
> > > seq, u64 rcd_sn)
> > >  
> > >  	rx_ctx = tls_offload_ctx_rx(tls_ctx);
> > >  	resync_req = atomic64_read(&rx_ctx->resync_req);
> > > -	req_seq = ntohl(resync_req >> 32) - ((u32)TLS_HEADER_SIZE - 1);
> > > +	req_seq = (resync_req >> 32) - ((u32)TLS_HEADER_SIZE - 1);  
> > 
> > this is not equivalent to what was before, 
> > resync_req is expected to be in network order, 
> > (TLS_HEADER_SIZE -1) is still in cpu indianness.
> 
> Naw, I think they are both in host order.
> 
> The driver passes network order.
> 
> But the stack has it in host order, this is the call site:
> 
> #ifdef CONFIG_TLS_DEVICE
> 	handle_device_resync(strp->sk, TCP_SKB_CB(skb)->seq + rxm-
> >offset,
> 			     *(u64*)tls_ctx->rx.rec_seq);
> #endif
> 
> The value passed by the driver used to be byte swapped when read from
> the atomic, but I moved the byte swap to when it's stored to the
> atomic.
> We used to have a weird situation where the atomic would have a be32
> on
> the top 32bits, and lower 32 bits would store the 1, in host order.
> 
> IOW the tls_offload_rx_resync_request() is the only thing setting
> this
> value and I moved the byte swap there.
> 
> $ git grep '.->resync_req'
> include/net/tls.h:      atomic64_set(&rx_ctx->resync_req,
> ((u64)ntohl(seq) << 32) | 1);
> net/tls/tls_device.c:   resync_req = atomic64_read(&rx_ctx-
> >resync_req);
> net/tls/tls_device.c:       atomic64_try_cmpxchg(&rx_ctx->resync_req, 
> &resync_req, 0))
> 
> Did I miss something or screw up tls_offload_rx_resync_request()?

No, after a second pass, i think all is good, i missed the fact in your
change now rx_ctx->resync_req is storing the value in host order.

Thanks,
Saeed.