RE: [PATCH net-next] tls: Fix improper revert in zerocopy_from_iter
> -Original Message-
> From: Doron Roberts-Kedes [mailto:doro...@fb.com]
> Sent: Wednesday, July 25, 2018 1:50 AM
> To: Vakul Garg
> Cc: David S . Miller ; Dave Watson
> ; Matt Mullins ;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net-next] tls: Fix improper revert in zerocopy_from_iter
>
> On Tue, Jul 24, 2018 at 05:13:26AM +, Vakul Garg wrote:
> >
> > > -Original Message-
> > > From: Doron Roberts-Kedes [mailto:doro...@fb.com]
> > > Sent: Tuesday, July 24, 2018 3:50 AM @@ -811,6 +809,7 @@ int
> > > tls_sw_recvmsg(struct sock *sk,
> > > likely(!(flags & MSG_PEEK))) {
> > > struct scatterlist sgin[MAX_SKB_FRAGS + 1];
> > > int pages = 0;
> > > + int orig_chunk = chunk;
> > >
> > > zc = true;
> > > sg_init_table(sgin, MAX_SKB_FRAGS + 1);
> @@ -820,9 +819,11 @@
> > > int tls_sw_recvmsg(struct sock *sk,
> > > err = zerocopy_from_iter(sk,
> > > >msg_iter,
> > >to_copy, ,
> > >, [1],
> > > - MAX_SKB_FRAGS,
> > > false, true);
> > > - if (err < 0)
> > > + MAX_SKB_FRAGS,
> > > false);
> > > + if (err < 0) {
> > > + iov_iter_revert(>msg_iter,
> > > chunk - orig_chunk);
> > > goto fallback_to_reg_recv;
> > > + }
> >
> > This assumes that msg_iter gets advanced even if zerocopy_from_iter()
> fails.
>
> Not sure I see what you mean. If msg_iter is not advanced then chunk -
> orig_chunk is 0, and the revert is a no-op.
>
As I said below, my comment was to improve code readability.
It takes a while to grasp that calling iov_iter_revert would result in no-op
if zerocopy_from_iter() fails.
> > It is easier from code readability perspective if functions upon failure do
> not leave any side effects for the caller to clean-up.
> > I suggest that iov_iter_revert() should be called from zerocopy_from_iter()
> itself if it is going to fail.
>
> Agreed that zerocopy_from_iter() should call iov_iter_revert(). I didn't do
> that because at first glace, the tx path seems to depend on the
> iov_iter_revert() being called as a result of either failed
> zerocopy_from_iter() or tls_push_record(). However, I think the latter path
> cannot actually be taken because tls_push_record appears never to return a
> positive value. This means that the code between the continue and
> fallback_to_reg_send should probably just be replaced with simply, goto
> send_end.
>
> After that change, its clear that iov_iter_revert() can be safely moved inside
> zerocopy_from_iter() in the error case.
>
> Pending your input, I'll plan on putting up a 2 part patch addressing each of
> these.
Please submit.
Re: [PATCH net-next] tls: Fix improper revert in zerocopy_from_iter
On Tue, Jul 24, 2018 at 05:13:26AM +, Vakul Garg wrote:
>
> > -Original Message-
> > From: Doron Roberts-Kedes [mailto:doro...@fb.com]
> > Sent: Tuesday, July 24, 2018 3:50 AM
> > @@ -811,6 +809,7 @@ int tls_sw_recvmsg(struct sock *sk,
> > likely(!(flags & MSG_PEEK))) {
> > struct scatterlist sgin[MAX_SKB_FRAGS + 1];
> > int pages = 0;
> > + int orig_chunk = chunk;
> >
> > zc = true;
> > sg_init_table(sgin, MAX_SKB_FRAGS + 1);
> > @@ -820,9 +819,11 @@ int tls_sw_recvmsg(struct sock *sk,
> > err = zerocopy_from_iter(sk,
> > >msg_iter,
> > to_copy, ,
> > , [1],
> > -MAX_SKB_FRAGS,
> > false, true);
> > - if (err < 0)
> > +MAX_SKB_FRAGS,
> > false);
> > + if (err < 0) {
> > + iov_iter_revert(>msg_iter,
> > chunk - orig_chunk);
> > goto fallback_to_reg_recv;
> > + }
>
> This assumes that msg_iter gets advanced even if zerocopy_from_iter() fails.
Not sure I see what you mean. If msg_iter is not advanced then chunk -
orig_chunk is 0, and the revert is a no-op.
> It is easier from code readability perspective if functions upon failure do
> not leave any side effects for the caller to clean-up.
> I suggest that iov_iter_revert() should be called from zerocopy_from_iter()
> itself if it is going to fail.
Agreed that zerocopy_from_iter() should call iov_iter_revert(). I didn't
do that because at first glace, the tx path seems to depend on the
iov_iter_revert() being called as a result of either failed
zerocopy_from_iter() or tls_push_record(). However, I think the latter path
cannot actually be taken because tls_push_record appears never to return
a positive value. This means that the code between
the continue and fallback_to_reg_send should probably just be replaced
with simply, goto send_end.
After that change, its clear that iov_iter_revert() can be safely moved
inside zerocopy_from_iter() in the error case.
Pending your input, I'll plan on putting up a 2 part patch addressing
each of these.
RE: [PATCH net-next] tls: Fix improper revert in zerocopy_from_iter
> -Original Message-
> From: Doron Roberts-Kedes [mailto:doro...@fb.com]
> Sent: Tuesday, July 24, 2018 3:50 AM
> To: David S . Miller
> Cc: Dave Watson ; Vakul Garg
> ; Matt Mullins ;
> netdev@vger.kernel.org; Doron Roberts-Kedes
> Subject: [PATCH net-next] tls: Fix improper revert in zerocopy_from_iter
>
> The current code is problematic because the iov_iter is reverted and never
> advanced in the non-error case. This patch skips the revert in the non-error
> case. This patch also fixes the amount by which the iov_iter is reverted.
> Currently, iov_iter is reverted by size, which can be greater than the amount
> by which the iter was actually advanced.
> Instead, mimic the tx path which reverts by the difference before and after
> zerocopy_from_iter.
>
> Fixes: 4718799817c5 ("tls: Fix zerocopy_from_iter iov handling")
> Signed-off-by: Doron Roberts-Kedes
> ---
> net/tls/tls_sw.c | 13 +++--
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index
> 490f2bcc6313..2ea000baebf8 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -276,7 +276,7 @@ static int zerocopy_from_iter(struct sock *sk, struct
> iov_iter *from,
> int length, int *pages_used,
> unsigned int *size_used,
> struct scatterlist *to, int to_max_pages,
> - bool charge, bool revert)
> + bool charge)
> {
> struct page *pages[MAX_SKB_FRAGS];
>
> @@ -327,8 +327,6 @@ static int zerocopy_from_iter(struct sock *sk, struct
> iov_iter *from,
> out:
> *size_used = size;
> *pages_used = num_elem;
> - if (revert)
> - iov_iter_revert(from, size);
>
> return rc;
> }
> @@ -431,7 +429,7 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr
> *msg, size_t size)
> >sg_plaintext_size,
> ctx->sg_plaintext_data,
> ARRAY_SIZE(ctx->sg_plaintext_data),
> - true, false);
> + true);
> if (ret)
> goto fallback_to_reg_send;
>
> @@ -811,6 +809,7 @@ int tls_sw_recvmsg(struct sock *sk,
> likely(!(flags & MSG_PEEK))) {
> struct scatterlist sgin[MAX_SKB_FRAGS + 1];
> int pages = 0;
> + int orig_chunk = chunk;
>
> zc = true;
> sg_init_table(sgin, MAX_SKB_FRAGS + 1);
> @@ -820,9 +819,11 @@ int tls_sw_recvmsg(struct sock *sk,
> err = zerocopy_from_iter(sk,
> >msg_iter,
>to_copy, ,
>, [1],
> - MAX_SKB_FRAGS,
> false, true);
> - if (err < 0)
> + MAX_SKB_FRAGS,
> false);
> + if (err < 0) {
> + iov_iter_revert(>msg_iter,
> chunk - orig_chunk);
> goto fallback_to_reg_recv;
> + }
This assumes that msg_iter gets advanced even if zerocopy_from_iter() fails.
It is easier from code readability perspective if functions upon failure do not
leave any side effects for the caller to clean-up.
I suggest that iov_iter_revert() should be called from zerocopy_from_iter()
itself if it is going to fail.
>
> err = decrypt_skb(sk, skb, sgin);
> for (; pages > 0; pages--)
> --
> 2.17.1
