On Tue, 27 Feb 2024 at 08:00, Steve Langasek <vorlon@xxxxxxxxxx> wrote:
>
> 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.
Debian sid.
> > > > +#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
> [...]
During my testing yesterday _FILE_OFFSET_BITS was not defined by
default (gcc-13/gcc-14/clang-18 on Debian sid).
> > > > +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
But isn't this the identical logic as for libselinux_ino_t?
If _FILE_OFFSET_BITS is defined to be 64 libselinux_ino_t is
typedef'ed to uint64_t and ino_t should also be a 64-bit integer.
If _FILE_OFFSET_BITS is not 64 libselinux_ino_t is typedef'ed to
uint32_t and ino_t should also be a 32-bit integer.
Also if I understand the patch idea correctly the 32-bit compatibility
type is only needed on 32-bit-LFS-enabled systems for the exported
symbol matchpathcon_filespec_add() to not break the ABI.
Changes to the header "only" lead to API breakage, which seems acceptable.
> > > > +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@xxxxxxxxxx vorlon@xxxxxxxxxx
