Re: [PATCH 1/5] packet: replace struct desc by struct iovec

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5626 bytes --]

On Thu, Jul 04, 2024 at 05:52:09PM +0200, Laurent Vivier wrote:
> On 24/06/2024 04:48, David Gibson wrote:
> > On Fri, Jun 21, 2024 at 04:56:36PM +0200, Laurent Vivier wrote:
> > 
> > Needs a commit message.
> > 
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > ---
> > >   packet.c | 75 +++++++++++++++++++++++++++++++-------------------------
> > >   packet.h | 14 ++---------
> > >   2 files changed, 43 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/packet.c b/packet.c
> > > index ccfc84607709..af2a539a1794 100644
> > > --- a/packet.c
> > > +++ b/packet.c
> ...
> > > +	}
> > > +
> > > +	if (start + len + offset > p->buf + p->buf_size) {
> > 
> > Also pre-existing, but I wonder if we should check for overflow of
> > (Start + len + offset).
> 
> Originally, I didn't want to change the existing behaviour. Only to move
> code, and to use a common function for packet_add_do() and packet_get_do().
> But if you think it should be better I can update the code for that:

Well, I think we should be more careful here, but as you say I don't
think it necessarily belongs as part of this series.

> > > +		if (func) {
> > > +			trace("packet offset plus length %lu from size %lu, "
> > > +			      "%s:%i", start - p->buf + len + offset,
> > > +			      p->buf_size, func, line);
> > > +		}
> > > +		return -1;
> > > +	}
> > > +
> > > +#if UINTPTR_MAX == UINT64_MAX
> > > +	if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) {
> > 
> > I don't think this check is relevant any more if we're going to iovecs
> > - this was just because the offset in struct desc was only 32-bit.
> 
> I agree.
> 
> > 
> > > +		trace("add packet start %p, buffer start %p, %s:%i",
> > > +		      (void *)start, (void *)p->buf, func, line);
> > > +		return -1;
> > > +	}
> > > +#endif
> > > +
> > > +	return 0;
> > > +}
> > >   /**
> > >    * packet_add_do() - Add data as packet descriptor to given pool
> > >    * @p:		Existing pool
> > > @@ -41,34 +71,16 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
> > >   		return;
> > >   	}
> > > -	if (start < p->buf) {
> > > -		trace("add packet start %p before buffer start %p, %s:%i",
> > > -		      (void *)start, (void *)p->buf, func, line);
> > > +	if (packet_check_range(p, 0, len, start, func, line))
> > >   		return;
> > > -	}
> > > -
> > > -	if (start + len > p->buf + p->buf_size) {
> > > -		trace("add packet start %p, length: %zu, buffer end %p, %s:%i",
> > > -		      (void *)start, len, (void *)(p->buf + p->buf_size),
> > > -		      func, line);
> > > -		return;
> > > -	}
> > >   	if (len > UINT16_MAX) {
> > >   		trace("add packet length %zu, %s:%i", len, func, line);
> > >   		return;
> > >   	}
> > > -#if UINTPTR_MAX == UINT64_MAX
> > > -	if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) {
> > > -		trace("add packet start %p, buffer start %p, %s:%i",
> > > -		      (void *)start, (void *)p->buf, func, line);
> > > -		return;
> > > -	}
> > > -#endif
> > > -
> > > -	p->pkt[idx].offset = start - p->buf;
> > > -	p->pkt[idx].len = len;
> > > +	p->pkt[idx].iov_base = (void *)start;
> > > +	p->pkt[idx].iov_len = len;
> > >   	p->count++;
> > >   }
> > > @@ -104,28 +116,23 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
> > >   		return NULL;
> > >   	}
> > > -	if (p->pkt[idx].offset + len + offset > p->buf_size) {
> > > +	if (len + offset > p->pkt[idx].iov_len) {
> > >   		if (func) {
> > > -			trace("packet offset plus length %zu from size %zu, "
> > > -			      "%s:%i", p->pkt[idx].offset + len + offset,
> > > -			      p->buf_size, func, line);
> > > +			trace("data length %zu, offset %zu from length %zu, "
> > > +			      "%s:%i", len, offset, p->pkt[idx].iov_len,
> > > +			      func, line);
> > 
> > I'm not sure either the old or new message is particularly descriptive
> > here :/
> 
> I think the func and line parameters will help to understand the problem,
> and the others why the trace is triggered.

Hmm, yeah, I guess so.

> > 
> > >   		}
> > >   		return NULL;
> > >   	}
> > > -	if (len + offset > p->pkt[idx].len) {
> > > -		if (func) {
> > > -			trace("data length %zu, offset %zu from length %u, "
> > > -			      "%s:%i", len, offset, p->pkt[idx].len,
> > > -			      func, line);
> > > -		}
> > > +	if (packet_check_range(p, offset, len, p->pkt[idx].iov_base,
> > > +			       func, line))
> > 
> > Ah.. right..  in this case we certainly don't want ASSERT()s in
> > packet_check_range().  Still wonder if that would make more sense for
> > the packet add case, however.
> > 
> > A couple of other points:
> >   * You've effectively switched the order of the two different tests here
> >     (one range checking against the entire buffer, one range checking
> >     against a single packet).  Any reason for that?
> 
> The idea is to check the parameters are valid before checking the buffer is valid.

Ok, makes sense.

> >   * Do we actually need the entire-buffer check here on the _get()
> >     side?  Isn't it enough to ensure that packets lie within the buffer
> >     when they're inserted?  Pre-existing, again, AFAICT.
> 
> I wanted to keep the idea introduced in bb708111833e ("treewide: Packet
> abstraction with mandatory boundary checks") and checking we don't read
> outside of the buffer.

Hm, ok.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]