Hi,
On Wed, Nov 20, 2024 at 11:08:12PM +0100, Salvatore Bonaccorso wrote:
> Hi Forian,
>
> Thanks for your time!
>
> On Wed, Nov 20, 2024 at 09:33:53PM +0100, Florian Weimer wrote:
> > * Salvatore Bonaccorso:
> >
> > > Hi Florian,
> > >
> > > On Wed, Nov 20, 2024 at 02:05:46PM +0100, Florian Weimer wrote:
> > >> * Salvatore Bonaccorso:
> > >>
> > >> > [ Changes ]
> > >> > Quoting the upsream commit is likely the best to explain the changes:
> > >> >
> > >> > | cr-restore: rseq: dynamically handle *libc with rseq
> > >> > | Before this patch we assumed that CRIU is compiled against
> > >> > | the same GLibc as it runs with. But as we see from real
> > >> > | world examples like #1935 it's not always true.
> > >> > |
> > >> > | The idea of this patch is to detect rseq configuration
> > >> > | for the main CRIU process and use it to unregister
> > >> > | rseq for all further child processes. It's correct,
> > >> > | because we restore pstree using clone*() syscalls,
> > >> > | don't use exec*() (!) syscalls, so rseq gets inherited
> > >> > | in the kernel and rseq configuration remains the same
> > >> > | for all children processes.
> > >>
> > >> There's are further commit you should consider picking up:
> > >>
> > >> commit 089345f77a34d1bc7ef146d650636afcd3cdda21
> > >> Author: Florian Weimer <fwei...@redhat.com>
> > >> Date: Wed Jul 10 18:34:50 2024 +0200
> > >>
> > >> Adjust to glibc __rseq_size semantic change
> > >>
> > >> In commit 2e456ccf0c34a056e3ccafac4a0c7effef14d918 ("Linux: Make
> > >> __rseq_size useful for feature detection (bug 31965)") glibc 2.40
> > >> changed the meaning of __rseq_size slightly: it is now the size
> > >> of the active/feature area (20 bytes initially), and not the size
> > >> of the entire initially defined struct (32 bytes including padding).
> > >> The reason for the change is that the size including padding does not
> > >> allow detection of newly added features while previously unused
> > >> padding is consumed.
> > >>
> > >> The prep_libc_rseq_info change in criu/cr-restore.c is not necessary
> > >> on kernels which have full ptrace support for obtaining rseq
> > >> information because the code is not used. On older kernels, it is
> > >> a correctness fix because with size 20 (the new value), rseq
> > >> registeration would fail.
> > >>
> > >> The two other changes are required to make rseq unregistration work
> > >> in tests.
> > >>
> > >> Signed-off-by: Florian Weimer <fwei...@redhat.com>
> > >
> > > Do you consider this optional, or required for the fix to land in
> > > bookrworm?
> >
> > The mentioned glibc change is probably in bookworm already. I think
> > it's this one from 2.36-9+deb12u8:
> >
> > - Fixes rseq extension mechanism.
> >
> > I think you need the test changes of the criu patch to get a clean
> > run. The non-test changes are dormant with sufficiently recent
> > kernels that contain full ptrace support.
>
> Which, if I got the bit rights, should be since around upstream
> 90f093fa8ea4 ("rseq, ptrace: Add PTRACE_GET_RSEQ_CONFIGURATION
> request") in v5.13-rc1.
>
> https://criu.org/Rseq
>
> Notabne: We do not run the testsuite during build for criu, and only
> recently it got added as autopkgtest, which makes the whole now way
> more confortable. For bookworm we almost have no coverage at all :-(
To get some more confidence I did run the test suite similarly we do
now in the unstable autopkgtest (plus additionally first patching the
rseq test with the additional commit):
./zdtm.py run --criu-bin=/usr/sbin/criu --crit-bin=/usr/bin/crit --keep-going
-a -x apparmor_stacking -x fd01 -x netns_lock_iptables
With that all tests pass (modulo the one skipped).
(Only on amd64 tested).
Regards,
Salvatore
