> -----Original Message-----
> From: netdev-ow...@vger.kernel.org [mailto:netdev-
> ow...@vger.kernel.org] On Behalf Of David Miller
> Sent: Thursday, July 26, 2018 1:59 AM
> To: Vakul Garg <vakul.g...@nxp.com>
> Cc: netdev@vger.kernel.org; bor...@mellanox.com;
> avia...@mellanox.com; davejwat...@fb.com
> Subject: Re: [PATCH net-next] net/tls: Corrected enabling of zero-copy mode
> 
> From: Vakul Garg <vakul.g...@nxp.com>
> Date: Mon, 23 Jul 2018 21:00:06 +0530
> 
> > @@ -787,7 +787,7 @@ int tls_sw_recvmsg(struct sock *sk,
> >     target = sock_rcvlowat(sk, flags & MSG_WAITALL, len);
> >     timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> >     do {
> > -           bool zc = false;
> > +           bool zc;
> >             int chunk = 0;
> >
> >             skb = tls_wait_data(sk, flags, timeo, &err);
>  ...
> > @@ -836,6 +835,7 @@ int tls_sw_recvmsg(struct sock *sk,
> >                             if (err < 0)
> >                                     goto fallback_to_reg_recv;
> >
> > +                           zc = true;
> >                             err = decrypt_skb_update(sk, skb, sgin, &zc);
> >                             for (; pages > 0; pages--)
> >                                     put_page(sg_page(&sgin[pages]));
> @@ -845,6 +845,7 @@ int
> > tls_sw_recvmsg(struct sock *sk,
> >                             }
> >                     } else {
> >  fallback_to_reg_recv:
> > +                           zc = false;
> >                             err = decrypt_skb_update(sk, skb, NULL,
> &zc);
> >                             if (err < 0) {
> >                                     tls_err_abort(sk, EBADMSG);
> > --
> > 2.13.6
> >
> 
> This will leave a code path where 'zc' is evaluated but not initialized to
> any value.
> 
> And that's the path taken when ctx->decrypted is true.  The code after
> your changes looks like:
> 
>               bool zc;
>  ...
>               if (!ctx->decrypted) {
> 
>  ... assignments to 'zc' happen in this code block
> 
>                       ctx->decrypted = true;
>               }
> 
>               if (!zc) {
> 
> So when ctx->decrypted it true, the if(!zc) condition runs on an
> uninitialized value.
> 
> I have to say that your TLS changes are becomming quite a time sink
> for two reasons.
> 
> First, you are making a lot of changes that seem not so needed, and
> whose value is purely determined by taste.  I'd put the
> msg_data_left() multiple evaluation patch into this category.
> 
> The rest require deep review and understanding of the complicated
> details of the TLS code, and many of them turn out to be incorrect.

My apologies for sending incorrect patches. I would be more careful next time. 


> 
> As I find more errors in your submissions, I begin to scrutinize your
> patches even more.  Thus, review of your changes takes even more time.
> 
> And it isn't helping that there are not a lot of other developers
> helping actively to review your changes.
> 
> I would like to just make a small request to you, that you concentrate
> on fixing clear bugs and clear issues that need to be resolved.
> 
> Thank you.

Reply via email to