Re: [PATCH] Always build for LFS mode on 32-bit archs.

[Steve Langasek <vorlon@debian.org>]

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

Thanks for the feedback.

On Mon, Feb 26, 2024 at 06:52:10PM +0100, Christian Göttsche wrote:
> > > +#include <stdint.h>

> I needed <asm/bitsperlong.h> aswell for __BITS_PER_LONG

Can you tell me about the platform you were building on?  In my tests, the
code with this patch behaved as intended on Ubuntu 24.04 pre-release with
glibc 2.38, on both armhf and amd64, with no additional includes required.

> > > +#if _FILE_OFFSET_BITS == 64

> One should probably check for existence:

> #if defined(_FILE_OFFSET_BITS) && _FILE_OFFSET_BITS == 64

Should we not consider it an error if this is not defined?  I can't think of
any reason why _FILE_OFFSET_BITS would legitimately be missing and in my
view it's ambiguous what we should do in this circumstance.  Using the
define without checking for existence first implicitly gives an error if
it's missing.  If preferred, we could be explicit as in:

#if !defined(_FILE_OFFSET_BITS)
#error [...]
#endif
#if _FILE_OFFSET_BITS == 64
[...]

> > > +typedef uint64_t libselinux_ino_t;
> > > +#if __BITS_PER_LONG < 64
> > > +#define matchpathcon_filespec_add matchpathcon_filespec_add64
> > > +#endif
> > > +#else
> > > +typedef uint32_t libselinux_ino_t;
> > > +#endif

> Is the typedef libselinux_ino_t really necessary, isn't it always just
> equal to ino_t?

When _FILE_OFFSET_BITS == 64, ino_t is a 64-bit type and we need to
explicitly declare a 32-bit type for the compatibility interface.  From
sys/stat.h:

# ifndef __ino_t_defined
#  ifndef __USE_FILE_OFFSET64
typedef __ino_t ino_t;
#  else
typedef __ino64_t ino_t;
#  endif
#  define __ino_t_defined
# endif

> > > +extern int matchpathcon_filespec_add(libselinux_ino_t ino, int specind, const char *file);
> > >
> > >  /* Destroy any inode associations that have been added, e.g. to restart
> > >     for a new filesystem. */
> > > diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
> > > index d3b981fc..267291aa 100644
> > > --- a/libselinux/src/Makefile
> > > +++ b/libselinux/src/Makefile
> > > @@ -87,6 +87,7 @@ CFLAGS ?= -O -Wall -W -Wundef -Wformat-y2k -Wformat-security -Winit-self -Wmissi
> > >            -fstack-protector-all --param=ssp-buffer-size=4 -fexceptions \
> > >            -fasynchronous-unwind-tables -fdiagnostics-show-option \
> > >            -Werror -Wno-aggregate-return \
> > > +          -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 \

> This won't work if CFLAGS are customized during build (e.g. by dpkg).
> Also utils/ is unaffected.
> Maybe use something like:

> libselinux/Makefile:
> 
> USE_LFS ?= y
> export USE_LFS
> 
> libselinux/{src,utils}/Makefile:
> 
> ifeq ($(USE_LFS),y)
>        override CFLAGS += -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
> endif

Thanks, I'm happy to amend the patch to do that (but will wait to converge
on the other points above before I resubmit).

I was largely unconcerned about the case of dpkg overriding it since, going
forward, dpkg would always be overriding it in the direction we wanted.  But
build systems for other distros may do otherwise and in that sense I agree
we should make it resistant to accidental clobbering.

> > > --- a/libselinux/src/libselinux.map
> > > +++ b/libselinux/src/libselinux.map
> > > @@ -251,4 +251,5 @@ LIBSELINUX_3.5 {
> > >    global:
> > >      getpidprevcon;
> > >      getpidprevcon_raw;
> > > +    matchpathcon_filespec_add64;
> > >  } LIBSELINUX_3.4;

> For downstream this seems fine, for upstream this should go into a new
> LIBSELINUX_3.6 section.

For me the ideal outcome is bidirectional ABI compatibility with upstream,
which means agreeing on the symbol version in advance before I upload this. 
For purposes of the upstream submission I'm happy to set this to
LIBSELINUX_3.6 (yes, obviously it wouldn't be LIBSELINUX_3.5 anymore since
that ABI is already fixed).  Barring a committment to land this change as
part of LIBSELINUX_3.6, my inclination for the downstream change this week
would be to leave it as an unversioned symbol so that consumers would be
forwards-compatible with later upstream inclusion regardless of the symbol
version used.

> > > diff --git a/libselinux/src/matchpathcon.c b/libselinux/src/matchpathcon.c
> > > index e44734c3..189e00fb 100644
> > > --- a/libselinux/src/matchpathcon.c
> > > +++ b/libselinux/src/matchpathcon.c
> > > @@ -195,7 +195,8 @@ static file_spec_t *fl_head;
> > >   * then use the specification that occurs later in the
> > >   * specification array.
> > >   */
> > > -int matchpathcon_filespec_add(ino_t ino, int specind, const char *file)
> > > +int matchpathcon_filespec_add(libselinux_ino_t ino, int specind,
> > > +                              const char *file)
> > >  {
> > >       file_spec_t *prevfl, *fl;
> > >       int h, ret;
> > > @@ -261,6 +262,18 @@ int matchpathcon_filespec_add(ino_t ino, int specind, const char *file)
> > >       return -1;
> > >  }
> > >
> > > +#if _FILE_OFFSET_BITS == 64 && __BITS_PER_LONG < 64
> > > +/* alias defined in the public header but we undefine it here */
> > > +#undef matchpathcon_filespec_add
> > > +
> > > +/* ABI backwards-compatible shim for non-LFS 32-bit systems */

> Missing extern declaration to avoid a missing-prototype warning.

Ack - will fix.

> > > +int matchpathcon_filespec_add(unsigned long ino, int specind,

> Are there 32-bit architectures we like to support where unsigned long
> is not 4 (but 8) bytes in size?

I am not aware of any. 
https://en.wikipedia.org/wiki/LP64#64-bit_data_models discusses the various
combinations of 32-bit and 64-bit types that are known to have been used,
and while there are configurations that use a 64-bit pointer with a 32-bit
long there are no examples shown for configurations that do the opposite.

-- 
Steve Langasek                   Give me a lever long enough and a Free OS
Debian Developer                   to set it on, and I can move the world.
Ubuntu Developer                                   https://www.debian.org/
slangasek@ubuntu.com                                     vorlon@debian.org

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