[LTP] [PATCH v1] Fix: dirtyc0w_shmem child process exit with error due to uninitialized lib_pid

[Li Wang]

On Mon, May 12, 2025 at 7:13 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > I think we'd better move them into a new structure dedicated to storing
> > library infrastructure.
>
> Sounds good.
>
> > > > As LTP library already provides a tst_reinit() function for child
> > > processes
> > > > spawned via exec()/execlp() to restore their test context.
> > > >
> > > > We could consider two approaches:
> > > >
> > > > 1. Create a separate IPC region to store infrastructure state(e.g.,
> > > > main_pid, lib_pid)
> > > > in a new struct tst_meta_info. The child can then access this data
> via
> > > > tst_reinit()
> > > > without modifying the struct results.
> > >
> > > I would like to keep a single IPC region because we have to pass a
> > > path to it to all children too.
> > >
> > > Also given that the minimal amount of memory we can allocate for IPC is
> > > a page we can as well have two structures, one for data and one for
> > > library infrastructure bits and place these structures in there
> > > manually. All we need to do is to make sure that we place them at
> > > aligned offsets.
> > >
> >
> > Yes, that was my thought as well. Something maybe like:
> >
> > #define LTP_MAGIC 0x4C54504D
>
> I suppose that we will check this in the tst_reinit() right? That is
> nice improvement.
>

Right, to validate the shared memory region. And the magic number
0x4C54504D is actually means "LTPM".


> > struct context {
> >         pid_t main_pid;
> >         pid_t lib_pid;
> >         struct timespec tst_start_time;
> >         /*
> >          * This is set by a call to tst_brk() with TBROK parameter and
> means
> >          * that the test should exit immediately.
> >          */
> >         unsigned int runtime;
> >         unsigned int overall_time;
> >         int abort_flag;
> >         int mntpoint_mounted;
> >         int ovl_mounted;
> >         int tdebug;
> > };
> >
> > struct results {
> >         int passed;
> >         int skipped;
> >         int failed;
> >         int warnings;
> >         int broken;
> > };
> >
> > struct ipc_block {
> >         unsigned int magic;
> >         struct context context;
> >         struct results results;
> >         futex_t futexes[];
> > };
>
> Maybe it would make sense to switch to the uint32_t and int32_t here
> when we are doing the cleanup. Currently we we call tst_reinit() from a
> 32bit process and the parent was 64bit it wouldn't work at all. We do
> not have tests like that at the moment but we may possibly end up in
> this situation later on.
>

Good point.


> > static struct ipc_block *ipc = NULL;
> > static struct context *context = NULL;
> > static struct results *results = NULL;
> >
> > #define TST_CONTEXT(ipc) ((struct context *)&(ipc)->context)
> > #define TST_RESULTS(ipc) ((struct results *)&(ipc)->results)
> > #define TST_FUTEXES(ipc) ((futex_t *)(ipc)->futexes)
>
> Is there a reason for these macros? The types should be correct, so
> there should be no need for the casts.
>

Yes, just to quickly get context/result address, but I will remove it if
it's not necessary.

New refactor patch (based on Wei's fix) is on the way.


-- 
Regards,
Li Wang