Re: [PATCH v3 2/3] eal: handle sysconf(_SC_PAGESIZE) negative return value

[Thomas Monjalon <thomas@monjalon.net>]

29/06/2025 00:49, Stephen Hemminger:
> On Sat, 28 Jun 2025 18:45:44 +0200
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Friday, 27 June 2025 20.30
> > > 
> > > 27/06/2025 19:49, Morten Brørup:  
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > Sent: Friday, 27 June 2025 19.35
> > > > >
> > > > > 27/06/2025 18:38, Morten Brørup:  
> > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > Sent: Friday, 27 June 2025 17.58
> > > > > > >
> > > > > > > 24/06/2025 10:03, Morten Brørup:  
> > > > > > > > +		if ((ssize_t)page_size < 0)
> > > > > > > > +			rte_panic("sysconf(_SC_PAGESIZE) failed: %s",
> > > > > > > > +					errno == 0 ? "Indeterminate" :  
> > > > > > > strerror(errno));
> > > > > > >
> > > > > > > We don't want more rte_panic().
> > > > > > > You could log the problem and return 0 here.
> > > > > > > It will be a problem later, but it may allow the application to  
> > > > > cleanup  
> > > > > > > instead of abrupting crashing.  
> > > > > >
> > > > > > Disagree.
> > > > > > That would be likely to cause crash with division by zero later.
> > > > > > Better to fail early.  
> > > > >
> > > > > Which division by zero?  
> > > >
> > > > Functions dividing by page size. E.g.:
> > > >  
> > > https://elixir.bootlin.com/dpdk/v25.03/source/lib/eal/common/eal_common_
> > > memory.c#L313  
> > > >  
> > > > >
> > > > > I don't think a library should take this decision on behalf of the  
> > > app.  
> > > >
> > > > I expect lots of things to break if sysconf(_SC_PAGESIZE) fails, so  
> > > the purpose of this patch is to centralize error handling here, and only
> > > continue/return with non-failing values.  
> > > >
> > > > Otherwise, everywhere using rte_mem_page_size() or  
> > > sysconf(_SC_PAGESIZE) should implement error handling (or ignore
> > > errors).  
> > > > That's a lot of places, so I'm not going to provide a patch doing  
> > > that.
> > > 
> > > I understand.
> > > 
> > > The problem is that we don't have an exception mechanism in this
> > > language.  
> > 
> > Yep.
> > And everyone assumes sysconf(_SC_PAGESIZE) never fails, which is probably correct, so nobody implemented error handling for it. Not even in rte_mem_page_size().
> > Coverity detected the missing error handling, and warns: "Although rte_mem_page_size() is declared to return unsigned int, it may actually return a negative value." This defect applies to all functions calling rte_mem_page_size().
> > This patch adds error handling to ensure that rte_mem_page_size() only returns non-negative values, or doesn’t return at all - i.e. fails with rte_panic() - so Coverity is satisfied with callers not implementing error handling for it.
> > 
> > It would be borderline waste of time fixing all the callers, so I fixed the root cause to satisfy Coverity.
> > 
> > From an higher level perspective:
> > This is a low level EAL function to determine the page size. I would consider it reasonable for such a low level EAL function to never fail.
> > If some O/S decides to not have a "system page size", and fail with "Indeterminate", e.g. to support multiple page sizes, we would need to handle that somehow. But let's ignore that until it actually happens, if ever.
> > 
> > If you are skeptical about this patch 2/3 in the series, we can escalate the discussion to the tech board. If you really hate this patch 2/3, I will honor a NAK from you. The patch is not important for me; I'm just trying to clean up.
> > 
> 
> In such cases, I look at glibc source and see if handles it or not.
> Looks like only used a couple of places there, the result of sysconf(_SC_PAGE_SIZE) is checked
> in one of the tests; but is not checked in the loading of locale's.  It expects a valid power of 2
> value there.
> 
> Ok to just die if value isn't valid.

Yes I'm convinced too.