Re: [PATCH v8 07/30] arp: Convert to iov_tail

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 7 Aug 2025 14:58:34 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 06/08/2025 04:17, David Gibson wrote:
> > On Tue, Aug 05, 2025 at 05:46:05PM +0200, Laurent Vivier wrote:  
> >> Use packet_data() and extract headers using IOV_REMOVE_HEADER()
> >> rather than packet_get().
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>  
> > 
> > Still R-b, but making an observation below that's perhaps more
> > relevant to the previous patch.
> >   
> >> ---
> >>   arp.c    | 12 +++++++++---
> >>   packet.c |  1 -
> >>   2 files changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arp.c b/arp.c
> >> index 9f1fedeafec0..b3ac42082841 100644
> >> --- a/arp.c
> >> +++ b/arp.c
> >> @@ -74,14 +74,20 @@ int arp(const struct ctx *c, const struct pool *p)
> >>   		struct arphdr ah;
> >>   		struct arpmsg am;
> >>   	} __attribute__((__packed__)) resp;
> >> +	struct arphdr ah_storage;
> >> +	struct ethhdr eh_storage;
> >> +	struct arpmsg am_storage;
> >>   	const struct ethhdr *eh;
> >>   	const struct arphdr *ah;
> >>   	const struct arpmsg *am;
> >> +	struct iov_tail data;
> >>   
> >> -	eh = packet_get(p, 0, 0,			 sizeof(*eh), NULL);
> >> -	ah = packet_get(p, 0, sizeof(*eh),		 sizeof(*ah), NULL);
> >> -	am = packet_get(p, 0, sizeof(*eh) + sizeof(*ah), sizeof(*am), NULL);
> >> +	if (!packet_data(p, 0, &data))
> >> +		return -1;  
> > 
> > The only case where packet_data() will return false is if you give it
> > a bad packet index.  That should never happen, by construction.  So
> > I'm wondering if that should be an ASSSERT() in packet_data() rather
> > than a return value.
> 
> Stefano, why do you think of this idea?

Well, yes, it *should* be by construction, but somewhere we might
eventually calculate that index (indirectly) using data we receive, and
I don't think we want to ASSERT() if somebody finds a way to make us
calculate a bad index.

It's not a strong objection against ASSERT(), though. It makes the code
marginally more terse and might help us find issues, too. I just have a
slight preference for a return value in this case anyway (better to
dodge a security issue and hide a functional issue than risking hitting
both, I think).

-- 
Stefano