On Thu, Aug 07, 2025 at 03:11:32PM +0200, Stefano Brivio wrote:
> 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.

I really can't imagine a scenario where we'd want to do that.  The
order of things in the packet pool is entirely arbitrary, so anything
from outside can't have any data that would be relevant to finding an
index.  So, in all cases we're either passing a (valid) index from one
part of our code to another, or scanning the entire pool.  Any failure
in either case would represent a pretty unlikely bug on our side,
which makes ASSERT() the appropriate choice IMO.

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

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