Re: [PATCH net] net/tls: fix sk_msg trim on fallback to copy mode

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Mon, 04 Nov 2019 10:56:52 -0800, John Fastabend wrote:
> Jakub Kicinski wrote:
> > diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> > index cf390e0aa73d..f87fde3a846c 100644
> > --- a/net/core/skmsg.c
> > +++ b/net/core/skmsg.c
> > @@ -270,18 +270,28 @@ void sk_msg_trim(struct sock *sk, struct sk_msg *msg, int len)
> >  
> >  	msg->sg.data[i].length -= trim;
> >  	sk_mem_uncharge(sk, trim);
> > +	/* Adjust copybreak if it falls into the trimmed part of last buf */
> > +	if (msg->sg.curr == i && msg->sg.copybreak > msg->sg.data[i].length)
> > +		msg->sg.copybreak = msg->sg.data[i].length;
> >  out:
> > -	/* If we trim data before curr pointer update copybreak and current
> > -	 * so that any future copy operations start at new copy location.
> > +	sk_msg_iter_var_next(i);
> > +	msg->sg.end = i;
> > +
> > +	/* If we trim data a full sg elem before curr pointer update
> > +	 * copybreak and current so that any future copy operations
> > +	 * start at new copy location.
> >  	 * However trimed data that has not yet been used in a copy op
> >  	 * does not require an update.
> >  	 */
> > -	if (msg->sg.curr >= i) {
> > +	if (!msg->sg.size) {
> > +		msg->sg.curr = msg->sg.start;
> > +		msg->sg.copybreak = 0;
> > +	} else if (sk_msg_iter_dist(msg->sg.start, msg->sg.curr) >
> > +		   sk_msg_iter_dist(msg->sg.end, msg->sg.curr)) {  
> 
> I'm not seeing how this can work. Taking simple case with start < end
> so normal geometry without wrapping. Let,
> 
>  start = 1
>  curr  = 3
>  end   = 4
> 
> We could trim an index to get,
> 
>  start = 1
>   curr = 3
>      i = 3
>    end = 4

IOW like this?

	test_one(/* start */ 1, /* curr */ 3, /* copybreak */ 150,
		 /* trim */ 500,
		 /* curr */ 3, /* copybreak */ 100, /* end */ 4,
		 /* data */ 200, 200, 200);

test #13  start:1 curr:3 end:4 cb:150 size: 600      0 200 200 200   0	OKAY

> Then after out: label this would push end up one,
> 
>  start = 1
>   curr = 3
>      i = 3
>    end = 4

I moved the assignment to end before the curr adjustment, so 'i' is
equivalent to 'end' at this point.

> But dist(start,curr) = 2 and dist(end, curr) = 1 and we would set curr
> to '3' but clear the copybreak?

I don't think we'd fall into this condition ever, unless we moved end.
And in your example AFAIU we don't move end.

> I think a better comparison would be,
> 
>   if (sk_msg_iter_dist(msg->sg.start, i) <
>       sk_msg_iter_dist(msg->sg.start, msg->sg.curr)
> 
> To check if 'i' walked past curr so we can reset curr/copybreak?

Ack, this does read better!

Should we use <= here? If we dropped a full segment, should curr point
at the end of the last remaining segment or should it point at 0 in end?