Date:   Fri, 28 Jun 2019 07:12:07 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     daniel@...earbox.io, ast@...nel.org, netdev@...r.kernel.org,
        edumazet@...gle.com, bpf@...r.kernel.org
Subject: Re: [PATCH 1/2] tls: remove close callback sock unlock/lock and
 flush_sync

Jakub Kicinski wrote:
> On Thu, 27 Jun 2019 10:36:42 -0700, John Fastabend wrote:
> > The tls close() callback currently drops the sock lock, makes a
> > cancel_delayed_work_sync() call, and then relocks the sock. This
> > seems suspect at best. The lock_sock() is applied to stop concurrent
> > operations on the socket while tearing the sock down. Further we
> > will need to add support for unhash() shortly and this complicates
> > matters because the lock may or may not be held then.
> > 
> > So to fix the above situation and simplify the next patch to add
> > unhash this patch creates a function tls_sk_proto_cleanup() that
> > tears down the socket without calling lock_sock/release_sock. In
> > order to flush the workqueue then we do the following,
> > 
> >   - Add a new bit to ctx, BIT_TX_CLOSING that is set when the
> >     tls resources are being removed.
> >   - Check this bit before scheduling any new work. This way we
> >     avoid queueing new work after tear down has started.
> >   - With the BIT_TX_CLOSING ensuring no new work is being added
> >     convert the cancel_delayed_work_sync to flush_delayed_work()
> >   - Finally call tlx_tx_records() to complete any available records
> >     before,
> >   - releasing and removing tls ctx.
> > 
> > The above is implemented for the software case namely any of
> > the following configurations from build_protos,
> > 
> >    prot[TLS_SW][TLS_BASE]
> >    prot[TLS_BASE][TLS_SW]
> >    prot[TLS_SW][TLS_SW]
> > 
> > The implication is a follow up patch is needed to resolve the
> > hardware offload case.
> > 
> > Tested with net selftests and bpf selftests.
> > 
> > Signed-off-by: John Fastabend <john.fastabend@...il.com>
> > ---
> >  include/net/tls.h  |    4 ++--
> >  net/tls/tls_main.c |   54 ++++++++++++++++++++++++++--------------------------
> >  net/tls/tls_sw.c   |   50 ++++++++++++++++++++++++++++++++----------------
> >  3 files changed, 62 insertions(+), 46 deletions(-)
> > 
> > diff --git a/include/net/tls.h b/include/net/tls.h
> > index 4a55ce6a303f..6fe1f5c96f4a 100644
> > --- a/include/net/tls.h
> > +++ b/include/net/tls.h
> > @@ -105,9 +105,7 @@ struct tls_device {
> >  enum {
> >  	TLS_BASE,
> >  	TLS_SW,
> > -#ifdef CONFIG_TLS_DEVICE
> >  	TLS_HW,
> > -#endif
> >  	TLS_HW_RECORD,
> >  	TLS_NUM_CONFIG,
> >  };
> > @@ -160,6 +158,7 @@ struct tls_sw_context_tx {
> >  	int async_capable;
> >  
> >  #define BIT_TX_SCHEDULED	0
> 
> BTW do you understand why we track this bit separately?  Just to avoid
> the irq operations in the workqueue code?
> 

Sorry not sure I understand. You mean vs simply scheduling the work
without checking the bit? Presumably its better to avoid scheduling
unnecessary work.

> > +#define BIT_TX_CLOSING		1
> 
> But since we do have the above, and I think it's tested everywhere,
> wouldn't setting SCHEDULED without accentually scheduling have
> effectively the same result?

It would block a send from calling tls_tx_records() but I guess that is
OK because this is a tear down operation and we are about to call
tls_tx_records anyways.

Sure we can do it this way might be slightly nicer to avoid checking
two bits.

> 
> >  	unsigned long tx_bitmask;
> >  };
> >  
> > @@ -327,6 +326,7 @@ void tls_sw_close(struct sock *sk, long timeout);
> >  void tls_sw_free_resources_tx(struct sock *sk);
> >  void tls_sw_free_resources_rx(struct sock *sk);
> >  void tls_sw_release_resources_rx(struct sock *sk);
> > +void tls_sw_release_strp_rx(struct tls_context *tls_ctx);
> >  int tls_sw_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> >  		   int nonblock, int flags, int *addr_len);
> >  bool tls_sw_stream_read(const struct sock *sk);
> > diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> > index fc81ae18cc44..51cb19e24dd9 100644
> > --- a/net/tls/tls_main.c
> > +++ b/net/tls/tls_main.c
> > @@ -261,24 +261,9 @@ static void tls_ctx_free(struct tls_context *ctx)
> >  	kfree(ctx);
> >  }
> >  
> > -static void tls_sk_proto_close(struct sock *sk, long timeout)
> > +static void tls_sk_proto_cleanup(struct sock *sk,
> > +				 struct tls_context *ctx, long timeo)
> >  {
> > -	struct tls_context *ctx = tls_get_ctx(sk);
> > -	long timeo = sock_sndtimeo(sk, 0);
> > -	void (*sk_proto_close)(struct sock *sk, long timeout);
> > -	bool free_ctx = false;
> > -
> > -	lock_sock(sk);
> > -	sk_proto_close = ctx->sk_proto_close;
> > -
> > -	if (ctx->tx_conf == TLS_HW_RECORD && ctx->rx_conf == TLS_HW_RECORD)
> > -		goto skip_tx_cleanup;
> > -
> > -	if (ctx->tx_conf == TLS_BASE && ctx->rx_conf == TLS_BASE) {
> > -		free_ctx = true;
> > -		goto skip_tx_cleanup;
> > -	}
> > -
> >  	if (!tls_complete_pending_work(sk, ctx, 0, &timeo))
> >  		tls_handle_open_record(sk, 0);
> >  
> > @@ -299,22 +284,37 @@ static void tls_sk_proto_close(struct sock *sk, long timeout)
> >  #ifdef CONFIG_TLS_DEVICE
> >  	if (ctx->rx_conf == TLS_HW)
> >  		tls_device_offload_cleanup_rx(sk);
> > -
> > -	if (ctx->tx_conf != TLS_HW && ctx->rx_conf != TLS_HW) {
> > -#else
> > -	{
> >  #endif
> > -		tls_ctx_free(ctx);
> > -		ctx = NULL;
> > +}
> > +
> > +static void tls_sk_proto_close(struct sock *sk, long timeout)
> > +{
> > +	struct tls_context *ctx = tls_get_ctx(sk);
> > +	long timeo = sock_sndtimeo(sk, 0);
> > +	void (*sk_proto_close)(struct sock *sk, long timeout);
> > +	bool free_ctx = false;
> 
> Set but not used?
> 

Removed in second patch but right should be removed here.

> > +
> > +	lock_sock(sk);
> > +	sk_proto_close = ctx->sk_proto_close;
> > +
> > +	if (ctx->tx_conf == TLS_HW_RECORD && ctx->rx_conf == TLS_HW_RECORD)
> > +		goto skip_tx_cleanup;
> > +
> > +	if (ctx->tx_conf == TLS_BASE && ctx->rx_conf == TLS_BASE) {
> > +		free_ctx = true;
> > +		goto skip_tx_cleanup;
> >  	}
> >  
> > +	tls_sk_proto_cleanup(sk, ctx, timeo);
> > +
> >  skip_tx_cleanup:
> >  	release_sock(sk);
> > +	if (ctx->rx_conf == TLS_SW)
> > +		tls_sw_release_strp_rx(ctx);
> >  	sk_proto_close(sk, timeout);
> > -	/* free ctx for TLS_HW_RECORD, used by tcp_set_state
> > -	 * for sk->sk_prot->unhash [tls_hw_unhash]
> > -	 */
> > -	if (free_ctx)
> > +
> > +	if (ctx->tx_conf != TLS_HW && ctx->rx_conf != TLS_HW &&
> > +	    ctx->tx_conf != TLS_HW_RECORD && ctx->rx_conf != TLS_HW_RECORD)
> >  		tls_ctx_free(ctx);
> >  }
> >  
> > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> > index 455a782c7658..d234a6b818e6 100644
> > --- a/net/tls/tls_sw.c
> > +++ b/net/tls/tls_sw.c
> > @@ -473,7 +473,8 @@ static void tls_encrypt_done(struct crypto_async_request *req, int err)
> >  		return;
> >  
> >  	/* Schedule the transmission */
> > -	if (!test_and_set_bit(BIT_TX_SCHEDULED, &ctx->tx_bitmask))
> > +	if (!test_and_set_bit(BIT_TX_SCHEDULED, &ctx->tx_bitmask) &&
> > +	    !test_bit(BIT_TX_CLOSING, &ctx->tx_bitmask))
> 
> Probably doesn't matter but seems like CLOSING test should be before
> the test_and_set().
> 

Yea, looks like we can drop CLOSING bit and use SCHEDULED bit makes
these a bit nicer.

> >  		schedule_delayed_work(&ctx->tx_work.work, 1);
> >  }
> >  
> > @@ -2058,16 +2059,26 @@ void tls_sw_free_resources_tx(struct sock *sk)
> >  	struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> >  	struct tls_rec *rec, *tmp;
> >  
> > +	/* Set TX CLOSING bit to stop tx_work from being scheduled
> > +	 * while tearing down TX context. We will flush any pending
> > +	 * work before free'ing ctx anyways. If already set then
> > +	 * another call is already free'ing resources.
> > +	 */
> 
> Oh, can we get multiple calls here?  Is this prep for unhash?
> 

It was prep for unhash() but there is a nicer way to get this so
we can drop it and just ensure we reset the prot callbacks before.

> > +	if (test_and_set_bit(BIT_TX_CLOSING, &ctx->tx_bitmask))
> > +		return;
> > +
> >  	/* Wait for any pending async encryptions to complete */
> >  	smp_store_mb(ctx->async_notify, true);
> >  	if (atomic_read(&ctx->encrypt_pending))
> >  		crypto_wait_req(-EINPROGRESS, &ctx->async_wait);
> >  
> > -	release_sock(sk);
> > -	cancel_delayed_work_sync(&ctx->tx_work.work);
> > -	lock_sock(sk);
> > -
> > -	/* Tx whatever records we can transmit and abandon the rest */
> > +	/* Flush work queue and then Tx whatever records we can
> > +	 * transmit and abandon the rest, lock_sock(sk) must be
> > +	 * held here. We ensure no further work is enqueue by
> > +	 * checking CLOSING bit before queueing new work and
> > +	 * setting it above.
> > +	 */
> > +	flush_delayed_work(&ctx->tx_work.work);
> >  	tls_tx_records(sk, -1);
> >  
> >  	/* Free up un-sent records in tx_list. First, free
> > @@ -2111,22 +2122,22 @@ void tls_sw_release_resources_rx(struct sock *sk)
> >  		write_lock_bh(&sk->sk_callback_lock);
> >  		sk->sk_data_ready = ctx->saved_data_ready;
> >  		write_unlock_bh(&sk->sk_callback_lock);
> > -		release_sock(sk);
> > -		strp_done(&ctx->strp);
> > -		lock_sock(sk);
> >  	}
> >  }
> >  
> > -void tls_sw_free_resources_rx(struct sock *sk)
> > +void tls_sw_release_strp_rx(struct tls_context *tls_ctx)
> >  {
> > -	struct tls_context *tls_ctx = tls_get_ctx(sk);
> >  	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
> >  
> > -	tls_sw_release_resources_rx(sk);
> > -
> > +	strp_done(&ctx->strp);
> >  	kfree(ctx);
> >  }
> >  
> > +void tls_sw_free_resources_rx(struct sock *sk)
> > +{
> > +	tls_sw_release_resources_rx(sk);
> > +}
> 
> I don't understand the RX side well enough, but perhaps a separate
> patch would make sense here?
> 

sure. Its actually its own fix I guess.

> >  /* The work handler to transmitt the encrypted records in tx_list */
> >  static void tx_work_handler(struct work_struct *work)
> >  {
> > @@ -2140,9 +2151,14 @@ static void tx_work_handler(struct work_struct *work)
> >  	if (!test_and_clear_bit(BIT_TX_SCHEDULED, &ctx->tx_bitmask))
> >  		return;
> >  
> > -	lock_sock(sk);
> > +	/* If we are running from a socket close operation then the
> > +	 * lock is already held so we do not need to hold it.
> > +	 */
> > +	if (likely(!test_bit(BIT_TX_CLOSING, &ctx->tx_bitmask)))
> > +		lock_sock(sk);
> 
> 	CPU 0 (free)		CPU 1 (wq)
> 				test_bit()
> 	lock(sk)
> 	set_bit()
> 				lock(sk)
> 	flush_work()
> 
> No?
> 

Yeah seems possible although never seen in my testing. So I'll
move the test_bit() inside the lock and do a ctx check to ensure
still have the reference.

  CPU 0 (free)           CPU 1 (wq)

  lock(sk)
                         lock(sk)
  set_bit()
  cancel_work()
  release
                         ctx = tls_get_ctx(sk)
                         unlikely(!ctx) <- we may have free'd 
                         test_bit()
                         ...
                         release()

or

  CPU 0 (free)           CPU 1 (wq)

                         lock(sk)
  lock(sk)
                         ctx = tls_get_ctx(sk)
                         unlikely(!ctx)
                         test_bit()
                         ...
                         release()
  set_bit()
  cancel_work()
  release

> >  	tls_tx_records(sk, -1);
> > -	release_sock(sk);
> > +	if (likely(!test_bit(BIT_TX_CLOSING, &ctx->tx_bitmask)))
> > +		release_sock(sk);
> >  }
> >  
> >  void tls_sw_write_space(struct sock *sk, struct tls_context *ctx)
> > @@ -2152,8 +2168,8 @@ void tls_sw_write_space(struct sock *sk, struct tls_context *ctx)
> >  	/* Schedule the transmission if tx list is ready */
> >  	if (is_tx_ready(tx_ctx) && !sk->sk_write_pending) {
> >  		/* Schedule the transmission */
> > -		if (!test_and_set_bit(BIT_TX_SCHEDULED,
> > -				      &tx_ctx->tx_bitmask))
> > +		if (!test_and_set_bit(BIT_TX_SCHEDULED, &tx_ctx->tx_bitmask) &&
> > +		    !test_bit(BIT_TX_CLOSING, &tx_ctx->tx_bitmask))
> >  			schedule_delayed_work(&tx_ctx->tx_work.work, 0);
> >  	}
> >  }
> > 
> 

Thanks,
John

Powered by blists - more mailing lists