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

[Li Wang]

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

> Hi!
> > > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > > index 2bb4519dd..b666715b9 100644
> > > --- a/lib/tst_test.c
> > > +++ b/lib/tst_test.c
> > > @@ -59,7 +59,7 @@ static const char *tid;
> > >  static int iterations = 1;
> > >  static float duration = -1;
> > >  static float timeout_mul = -1;
> > > -static pid_t main_pid, lib_pid;
> > > +static pid_t lib_pid;
> > >  static int mntpoint_mounted;
> > >  static int ovl_mounted;
> > >  static struct timespec tst_start_time; /* valid only for test pid */
> > > @@ -78,6 +78,8 @@ struct results {
> > >         int abort_flag;
> > >         unsigned int runtime;
> > >         unsigned int overall_time;
> > > +       pid_t lib_pid;
> > > +       pid_t main_pid;
> > >  };
> >
> > Can we avoid polluting the struct results with main_pid and lib_pid?
> > From a design separation standpoint, results should only store test
> > outcome data, not test infrastructure state.
>
> We do have the abort_flag and runtime there and checkpoints use the
> memory after this structure as well, so I wouldn't be so strict.
>

I think we'd better move them into a new structure dedicated to storing
library infrastructure.


>
> > 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

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;
};

static struct ipc_block {
        unsigned int magic;
        struct context context;
        struct results results;
        futex_t futexes[];
};

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)

...



>
> > 2. Simply pass main_pid and lib_pid through environment variables in the
> > ltp library, and retrieve them from tst_reinit() in the child.
>
> That would work too, but this adds more complexity.
>
> > Or, maybe others have simpler options. Cc'ing them.
> >
> > In either case, we should set 'tst_test->child_need_reinit' in the
> parent.
> >
> > @Cyril, @Petr, I support merging this fix before the May release, as
> I’ve also
> > encountered the same failure during my pre-release testing.
>
> Yes, please, it's good enough even if it's going to be a temporary
> solution.
>

Sure, we can merge this one. And later, I will work on a new patch
to refactor the structure like above.

Reviewed-by: Li Wang <liwang@redhat.com>



>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>
>

-- 
Regards,
Li Wang