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

[Laurent Vivier <lvivier@redhat.com>]

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:

>> +		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.

> 
>>   		}
>>   		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.

>   * 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.

Thanks,
Laurent