> -----Original Message-----
> From: Doron Roberts-Kedes [mailto:doro...@fb.com]
> Sent: Wednesday, July 25, 2018 1:50 AM
> To: Vakul Garg <vakul.g...@nxp.com>
> Cc: David S . Miller <da...@davemloft.net>; Dave Watson
> <davejwat...@fb.com>; Matt Mullins <mmull...@fb.com>;
> 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 +0000, 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-
> > > >msg_iter,
> > > to_copy, &pages,
> > > &chunk, &sgin[1],
> > > - MAX_SKB_FRAGS,
> > > false, true);
> > > - if (err < 0)
> > > + MAX_SKB_FRAGS,
> > > false);
> > > + if (err < 0) {
> > > + iov_iter_revert(&msg->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.
