Message ID | 20190522020202.4792-2-jakub.kicinski@netronome.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | net/tls: fix device surprise removal with offload | expand |
On Tue, 21 May 2019 19:02:00 -0700, Jakub Kicinski wrote: > When netdev with active kTLS sockets in unregistered > notifier callback walks the offloaded sockets and > cleans up offload state. RX data may still be processed, > however, and if resync was requested prior to device > removal we would hit a NULL pointer dereference on > ctx->netdev use. > > Make sure resync is under the device offload lock > and NULL-check the netdev pointer. > > This should be safe, because the pointer is set to > NULL either in the netdev notifier (under said lock) > or when socket is completely dead and no resync can > happen. > > The other access to ctx->netdev in tls_validate_xmit_skb() > does not dereference the pointer, it just checks it against > other device pointer, so it should be pretty safe (perhaps > we can add a READ_ONCE/WRITE_ONCE there, if paranoid). > > Fixes: 4799ac81e52a ("tls: Add rx inline crypto offload") > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com> > --- > net/tls/tls_device.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c > index ca54a7c7ec81..aa33e4accc32 100644 > --- a/net/tls/tls_device.c > +++ b/net/tls/tls_device.c > @@ -553,8 +553,8 @@ void tls_device_write_space(struct sock *sk, struct tls_context *ctx) > void handle_device_resync(struct sock *sk, u32 seq, u64 rcd_sn) > { > struct tls_context *tls_ctx = tls_get_ctx(sk); > - struct net_device *netdev = tls_ctx->netdev; > struct tls_offload_context_rx *rx_ctx; > + struct net_device *netdev; > u32 is_req_pending; > s64 resync_req; > u32 req_seq; > @@ -568,10 +568,15 @@ void handle_device_resync(struct sock *sk, u32 seq, u64 rcd_sn) > is_req_pending = resync_req; > > if (unlikely(is_req_pending) && req_seq == seq && > - atomic64_try_cmpxchg(&rx_ctx->resync_req, &resync_req, 0)) > - netdev->tlsdev_ops->tls_dev_resync_rx(netdev, sk, > - seq + TLS_HEADER_SIZE - 1, > - rcd_sn); > + atomic64_try_cmpxchg(&rx_ctx->resync_req, &resync_req, 0)) { > + seq += TLS_HEADER_SIZE - 1; > + down_read(&device_offload_lock); Sorry this may actually cause a sleep in atomic, turns out resync may get called directly from softirq under certain conditions. Would it be possible to drop this from stable? I can post a revert + new fix (probably on a refcount..) or should I post an incremental fix? > + netdev = tls_ctx->netdev; > + if (netdev) > + netdev->tlsdev_ops->tls_dev_resync_rx(netdev, sk, seq, > + rcd_sn); > + up_read(&device_offload_lock); > + } > } > > static int tls_device_reencrypt(struct sock *sk, struct sk_buff *skb)
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index ca54a7c7ec81..aa33e4accc32 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -553,8 +553,8 @@ void tls_device_write_space(struct sock *sk, struct tls_context *ctx) void handle_device_resync(struct sock *sk, u32 seq, u64 rcd_sn) { struct tls_context *tls_ctx = tls_get_ctx(sk); - struct net_device *netdev = tls_ctx->netdev; struct tls_offload_context_rx *rx_ctx; + struct net_device *netdev; u32 is_req_pending; s64 resync_req; u32 req_seq; @@ -568,10 +568,15 @@ void handle_device_resync(struct sock *sk, u32 seq, u64 rcd_sn) is_req_pending = resync_req; if (unlikely(is_req_pending) && req_seq == seq && - atomic64_try_cmpxchg(&rx_ctx->resync_req, &resync_req, 0)) - netdev->tlsdev_ops->tls_dev_resync_rx(netdev, sk, - seq + TLS_HEADER_SIZE - 1, - rcd_sn); + atomic64_try_cmpxchg(&rx_ctx->resync_req, &resync_req, 0)) { + seq += TLS_HEADER_SIZE - 1; + down_read(&device_offload_lock); + netdev = tls_ctx->netdev; + if (netdev) + netdev->tlsdev_ops->tls_dev_resync_rx(netdev, sk, seq, + rcd_sn); + up_read(&device_offload_lock); + } } static int tls_device_reencrypt(struct sock *sk, struct sk_buff *skb)