| Message ID | 20230614062212.73288-5-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/tls: fixes for NVMe-over-TLS | expand |
On Wed, 14 Jun 2023 08:22:12 +0200 Hannes Reinecke wrote:
> Implement ->read_sock() function for use with nvme-tcp.
please take tls_rx_reader_lock() :S
+ Dan Carpenter On Wed, Jun 14, 2023 at 08:22:12AM +0200, Hannes Reinecke wrote: ... > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index 47eeff4d7d10..f0e0a0afb8c9 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -2231,6 +2231,77 @@ ssize_t tls_sw_splice_read(struct socket *sock, loff_t *ppos, > goto splice_read_end; > } > > +int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc, > + sk_read_actor_t read_actor) > +{ > + struct tls_context *tls_ctx = tls_get_ctx(sk); > + struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx); > + struct strp_msg *rxm = NULL; > + struct tls_msg *tlm; > + struct sk_buff *skb; > + ssize_t copied = 0; > + int err, used; > + > + if (!skb_queue_empty(&ctx->rx_list)) { > + skb = __skb_dequeue(&ctx->rx_list); > + } else { > + struct tls_decrypt_arg darg; > + > + err = tls_rx_rec_wait(sk, NULL, true, true); > + if (err <= 0) > + return err; > + > + memset(&darg.inargs, 0, sizeof(darg.inargs)); > + > + err = tls_rx_one_record(sk, NULL, &darg); > + if (err < 0) { > + tls_err_abort(sk, -EBADMSG); > + return err; > + } > + > + tls_rx_rec_done(ctx); > + skb = darg.skb; > + } > + > + do { > + rxm = strp_msg(skb); > + tlm = tls_msg(skb); > + > + /* read_sock does not support reading control messages */ > + if (tlm->control != TLS_RECORD_TYPE_DATA) { > + err = -EINVAL; > + goto read_sock_requeue; > + } > + > + used = read_actor(desc, skb, rxm->offset, rxm->full_len); > + if (used <= 0) { > + err = used; > + goto read_sock_end; > + } > + > + copied += used; > + if (used < rxm->full_len) { > + rxm->offset += used; > + rxm->full_len -= used; > + if (!desc->count) > + goto read_sock_requeue; > + } else { > + consume_skb(skb); > + if (desc->count && !skb_queue_empty(&ctx->rx_list)) > + skb = __skb_dequeue(&ctx->rx_list); > + else > + skb = NULL; > + } > + } while (skb); > + > +read_sock_end: > + return copied ? : err; Hi Hannes, I'm of two minds about raising this or not, but in any case here I am doing so. Both gcc-12 [-Wmaybe-uninitialized] and Smatch warn that err may be used uninitialised on the line above. My own analysis is that it cannot occur: I think it is always the case that either copied is non-zero or err is initialised. But still the warning is there. And in future it may create noise that may crowds out real problems. It also seems to imply that the path is somewhat complex, and hard to analyse: certainly it took my small brain a while. So I do wonder if there is a value in ensuring err is always set to something appropriate, perhaps set to -EINVAL above the do loop. I guess that in the end I decided it was best to put this thinking in the open. And let you decide. > + > +read_sock_requeue: > + __skb_queue_head(&ctx->rx_list, skb); > + goto read_sock_end; > +} > + > bool tls_sw_sock_is_readable(struct sock *sk) > { > struct tls_context *tls_ctx = tls_get_ctx(sk); > -- > 2.35.3 > >
On Sat, Jun 17, 2023 at 04:08:08PM +0200, Simon Horman wrote: > + Dan Carpenter > > On Wed, Jun 14, 2023 at 08:22:12AM +0200, Hannes Reinecke wrote: > > ... > > > + > > +read_sock_end: > > + return copied ? : err; > > Hi Hannes, > > I'm of two minds about raising this or not, but in any case here I am > doing so. > > Both gcc-12 [-Wmaybe-uninitialized] and Smatch warn that err may be > used uninitialised on the line above. > > My own analysis is that it cannot occur: I think it is always the case > that either copied is non-zero or err is initialised. Hm... Yeah. This is a bug in how Smatch handles: return copied ? : err; Smatch wants every return to have a simple literal or variable. So if the return is complicated it gets changed into: fake_variable = copied ? : err; return fake_variable; Smatch translates this to: if (!(fake_variable = copied)) fake_variable = err; [ Here fake_variable doesn't have side effects but under other circumstances this transformation could cause double evaluate side effects bugs. So that's another bug in Smatch. ] Then Smatch parses the fake_variable = copied condition as: fake_variable = copied; if (fake_variable) { The problem is that the type of fake_variable is based on the type of tls_sw_read_sock() so it is a 32bit while copied is a 64bit (of unknown value). So Smatch says, "Just because copied is non-zero doesn't mean fake_variable is non-zero because the value might get truncated when we cast away the high 32 bits." This not a serious proposal but a just to demonstrate that this is what happens there are two ways to silence this warning. Changing the type of tls_sw_read_sock() to long. Or change the code to: if (copied) return copied; return err; Probably the right thing is to create a second fake_copied variable based on typeof(copied). fake_copied = copied; if (fake_copied) fake_return_variable = fake_copied; else fake_return_variable = err; It's a doable thing. Plus now there are no double evaluate side effects bugs. I have written this code and it silences the warning, but I'll test it out tonight to see what breaks. regards, dan carpenter
diff --git a/net/tls/tls.h b/net/tls/tls.h index d002c3af1966..ba55cd5c4913 100644 --- a/net/tls/tls.h +++ b/net/tls/tls.h @@ -114,6 +114,8 @@ bool tls_sw_sock_is_readable(struct sock *sk); ssize_t tls_sw_splice_read(struct socket *sock, loff_t *ppos, struct pipe_inode_info *pipe, size_t len, unsigned int flags); +int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc, + sk_read_actor_t read_actor); int tls_device_sendmsg(struct sock *sk, struct msghdr *msg, size_t size); void tls_device_splice_eof(struct socket *sock); diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c index 7b9c83dd7de2..1a062a8c6d33 100644 --- a/net/tls/tls_main.c +++ b/net/tls/tls_main.c @@ -963,10 +963,12 @@ static void build_proto_ops(struct proto_ops ops[TLS_NUM_CONFIG][TLS_NUM_CONFIG] ops[TLS_BASE][TLS_SW ] = ops[TLS_BASE][TLS_BASE]; ops[TLS_BASE][TLS_SW ].splice_read = tls_sw_splice_read; ops[TLS_BASE][TLS_SW ].poll = tls_sk_poll; + ops[TLS_BASE][TLS_SW ].read_sock = tls_sw_read_sock; ops[TLS_SW ][TLS_SW ] = ops[TLS_SW ][TLS_BASE]; ops[TLS_SW ][TLS_SW ].splice_read = tls_sw_splice_read; ops[TLS_SW ][TLS_SW ].poll = tls_sk_poll; + ops[TLS_SW ][TLS_SW ].read_sock = tls_sw_read_sock; #ifdef CONFIG_TLS_DEVICE ops[TLS_HW ][TLS_BASE] = ops[TLS_BASE][TLS_BASE]; diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index 47eeff4d7d10..f0e0a0afb8c9 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -2231,6 +2231,77 @@ ssize_t tls_sw_splice_read(struct socket *sock, loff_t *ppos, goto splice_read_end; } +int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc, + sk_read_actor_t read_actor) +{ + struct tls_context *tls_ctx = tls_get_ctx(sk); + struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx); + struct strp_msg *rxm = NULL; + struct tls_msg *tlm; + struct sk_buff *skb; + ssize_t copied = 0; + int err, used; + + if (!skb_queue_empty(&ctx->rx_list)) { + skb = __skb_dequeue(&ctx->rx_list); + } else { + struct tls_decrypt_arg darg; + + err = tls_rx_rec_wait(sk, NULL, true, true); + if (err <= 0) + return err; + + memset(&darg.inargs, 0, sizeof(darg.inargs)); + + err = tls_rx_one_record(sk, NULL, &darg); + if (err < 0) { + tls_err_abort(sk, -EBADMSG); + return err; + } + + tls_rx_rec_done(ctx); + skb = darg.skb; + } + + do { + rxm = strp_msg(skb); + tlm = tls_msg(skb); + + /* read_sock does not support reading control messages */ + if (tlm->control != TLS_RECORD_TYPE_DATA) { + err = -EINVAL; + goto read_sock_requeue; + } + + used = read_actor(desc, skb, rxm->offset, rxm->full_len); + if (used <= 0) { + err = used; + goto read_sock_end; + } + + copied += used; + if (used < rxm->full_len) { + rxm->offset += used; + rxm->full_len -= used; + if (!desc->count) + goto read_sock_requeue; + } else { + consume_skb(skb); + if (desc->count && !skb_queue_empty(&ctx->rx_list)) + skb = __skb_dequeue(&ctx->rx_list); + else + skb = NULL; + } + } while (skb); + +read_sock_end: + return copied ? : err; + +read_sock_requeue: + __skb_queue_head(&ctx->rx_list, skb); + goto read_sock_end; +} + bool tls_sw_sock_is_readable(struct sock *sk) { struct tls_context *tls_ctx = tls_get_ctx(sk);