|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH] lib/ukboot: Fix handling of main arguments
Hi Simon,
Please see inline.
On 6/6/19 3:34 PM, Simon Kuenzer wrote:
> Hey Costin,
>
> On 06.06.19, 12:30, "Costin Lupu" <costin.lup@xxxxxxxxx> wrote:
>
> Hi Simon,
>
> I've read your comments but I couldn't find out why you don't agree with
> global approach. So, what are the reasons why you would prefer declaring
> the arguments static rather than global?
>
> The change that I mention as alternative is minimal and less invasive to the
> existing code. You do not need to add it as exported symbol and most code can
> stay as is. I also do not see why we would need the global approach. Do you
> have a use case? I think this is conflicting how argc and argv is intended to
> be used: The idea about them is that they are private to your main() function
> of your program (ukplat_entry() in our case). It can do whatever with it and
> this does not have any effect on something else.
> As an example you can take getopt(): It starts re-ordering the pointers on
> argv for doing its argument parsing.
> On a global and shared data structure you would not want this.
Yeah, that's what I was looking for. Thanks for the explanations. I'll
send a v2 making the argv array static.
>
> Personally I find making the arguments global to be more elegant and
> offer a unified approach, since they would have the same location for
> all platforms.
>
> Also, please see a comment inline.
>
> On 6/6/19 12:34 PM, Simon Kuenzer wrote:
> > Hey Costin,
> >
> > thanks a lot for the patch. I think the actual problem is that the boot
> > stack could be released later after boot for something else, so it is
> > not guaranteed anymore that the arguments are safe and staying.
> > However, I don't agree to make argv and argc global available. Just
> > allocate them in the bss section.
> > See my comments inline.
> >
> > On 05.06.19 21:55, Costin Lupu wrote:
> >> When the command line is parsed and split into arguments, each argument
> >> is saved in an array of pointers allocated on the stack ('argv' array
> in
> >> ukplat_entry_argp()). This means that the array can be overwritten any
> >> time before starting the main thread which needs the arguments. This
> >> patch fixes that by making the array global, encapsulated by the
> >> 'main_arg' structure.
> >>
> >> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> >> ---
> >> include/uk/plat/bootstrap.h | 11 ++++++++++-
> >> lib/ukboot/boot.c | 33 ++++++++++++---------------------
> >> lib/ukboot/exportsyms.uk | 1 +
> >> plat/linuxu/setup.c | 7 ++++++-
> >> 4 files changed, 29 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/include/uk/plat/bootstrap.h b/include/uk/plat/bootstrap.h
> >> index 0652ccd1..a9e404e8 100644
> >> --- a/include/uk/plat/bootstrap.h
> >> +++ b/include/uk/plat/bootstrap.h
> >> @@ -36,6 +36,7 @@
> >> #ifndef __UKPLAT_BOOTSTRAP__
> >> #define __UKPLAT_BOOTSTRAP__
> >> +#include <uk/config.h>
> >> #include <uk/arch/types.h>
> >> #include <uk/essentials.h>
> >> @@ -43,6 +44,14 @@
> >> extern "C" {
> >> #endif
> >> +/* Main arguments structure */
> >> +struct main_arg {
> >> + int argc;
> >> + char *argv[CONFIG_LIBUKBOOT_MAXNBARGS];
> >> +};
> >> +/* Main arguments */
> >> +extern struct main_arg global_main_arg;
> >> +
> >> /**
> >> * Called by platform library during initialization
> >> * This function has to be provided by a non-platform library for
> >> bootstrapping
> >> @@ -52,7 +61,7 @@ extern "C" {
> >> * @param argc Number of arguments
> >> * @param args Array to '\0'-terminated arguments
> >> */
> >> -void ukplat_entry(int argc, char *argv[]) __noreturn;
> >> +void ukplat_entry(struct main_arg *arg) __noreturn;
> >> /**
> >> * Called by platform library during initialization
> >> diff --git a/lib/ukboot/boot.c b/lib/ukboot/boot.c
> >> index 4dcc0afb..5cc3b573 100644
> >> --- a/lib/ukboot/boot.c
> >> +++ b/lib/ukboot/boot.c
> >> @@ -68,17 +68,13 @@ extern int liblwip_init(void);
> >> #endif /* CONFIG_LIBLWIP */
> >> static void main_thread_func(void *arg) __noreturn;
> >> -
> >> -struct thread_main_arg {
> >> - int argc;
> >> - char **argv;
> >> -};
> >> +struct main_arg global_main_arg;
> >> static void main_thread_func(void *arg)
> >> {
> >> int i;
> >> int ret;
> >> - struct thread_main_arg *tma = arg;
> >> + struct main_arg *tma = arg;
> >> uk_pr_info("Pre-init table at %p - %p\n",
> >> __preinit_array_start, &__preinit_array_end);
> >> @@ -145,26 +141,26 @@ static void main_thread_func(void *arg)
> >> /* defined in <uk/plat.h> */
> >> void ukplat_entry_argp(char *arg0, char *argb, __sz argb_len)
> >> {
> >> - char *argv[CONFIG_LIBUKBOOT_MAXNBARGS];
> >
> > Just declare
> > char *argv[CONFIG_LIBUKBOOT_MAXNBARGS];
> > as
> > static char *argv[CONFIG_LIBUKBOOT_MAXNBARGS];
> >
> > The same for argc.
>
> Actually, we do not need to declare `argc` as static, only `argv`. I was
> wrong. `argc` is anyways handed over by-value on a function call, not
> by-reference (like argv) - so we are good.
>
> >
> > We also need to double-check that our string buffer with the arguments
> > (where argv is pointing to), is not allocated on the stack in the
> > platform. If this is the case, we are safe.
> >
>
> I'm not sure I follow this, you mean to check if the strings aren't
> allocated on this stack? Or to check if it comes from bss?
>
> In case the boot stack is released or used for something else, there should
> be no reference to it. The buffer where the strings are stored (and where the
> pointers of argv are pointing to) shouldn't be on the stack either. I
> mentioned this because I wanted to say that we should double check that our
> platform are storing this buffer on something else than the stack.
>
I'd rather not add this to the next version because it needs substantial
changes in order to access the bootstack.
> >> - int argc = 0;
> >> + global_main_arg.argc = 0;
> >> if (arg0) {
> >> - argv[0] = arg0;
> >> - argc += 1;
> >> + global_main_arg.argv[0] = arg0;
> >> + global_main_arg.argc += 1;
> >> }
> >> if (argb && argb_len) {
> >> - argc += uk_argnparse(argb, argb_len, arg0 ? &argv[1] :
> &argv[0],
> >> + global_main_arg.argc += uk_argnparse(argb, argb_len,
> >> + arg0 ? &global_main_arg.argv[1]
> >> + : &global_main_arg.argv[0],
> >> arg0 ? (CONFIG_LIBUKBOOT_MAXNBARGS - 1)
> >> : CONFIG_LIBUKBOOT_MAXNBARGS);
> >> }
> >> - ukplat_entry(argc, argv);
> >> + ukplat_entry(&global_main_arg);
> >
> > If you declare the arguments as global you do not need to hand them over
> > anymore, but as I said I would not change this at all.
> >
> >> }
> >> /* defined in <uk/plat.h> */
> >> -void ukplat_entry(int argc, char *argv[])
> >> +void ukplat_entry(struct main_arg *arg)
> >> {
> >> const uk_ctor_func_t *cfn;
> >> - struct thread_main_arg tma;
> >> #if CONFIG_LIBUKALLOC
> >> struct uk_alloc *a = NULL;
> >> int rc;
> >> @@ -234,19 +230,14 @@ void ukplat_entry(int argc, char *argv[])
> >> s = uk_sched_default_init(a);
> >> if (unlikely(!s))
> >> UK_CRASH("Could not initialize the scheduler\n");
> >> -#endif
> >> - tma.argc = argc;
> >> - tma.argv = argv;
> >> -
> >> -#if CONFIG_LIBUKSCHED
> >> - main_thread = uk_thread_create("main", main_thread_func, &tma);
> >> + main_thread = uk_thread_create("main", main_thread_func, arg);
> >> if (unlikely(!main_thread))
> >> UK_CRASH("Could not create main thread\n");
> >> uk_sched_start(s);
> >> #else
> >> /* Enable interrupts before starting the application */
> >> ukplat_lcpu_enable_irq();
> >> - main_thread_func(&tma);
> >> + main_thread_func(arg);
> >> #endif
> >> }
> >> diff --git a/lib/ukboot/exportsyms.uk b/lib/ukboot/exportsyms.uk
> >> index 3edc6c6a..adbb84fb 100644
> >> --- a/lib/ukboot/exportsyms.uk
> >> +++ b/lib/ukboot/exportsyms.uk
> >> @@ -1,3 +1,4 @@
> >> ukplat_entry_argp
> >> ukplat_entry
> >> +global_main_arg
> >> main
> >> diff --git a/plat/linuxu/setup.c b/plat/linuxu/setup.c
> >> index 50454437..4f6f5688 100644
> >> --- a/plat/linuxu/setup.c
> >> +++ b/plat/linuxu/setup.c
> >> @@ -172,6 +172,11 @@ void _liblinuxuplat_entry(int argc, char *argv[])
> >> argv += (ret - 1);
> >> argv[0] = progname;
> >
> > Make also sure that argv and argc are declared as static here, instead.
> >
> >> + /* Setup global arguments */
> >> + global_main_arg.argc = argc;
> >> + for (int i = 0; i < argc; i++)
> >> + global_main_arg.argv[i] = argv[i];
> >> +
> >> /*
> >> * Allocate heap memory
> >> */
> >> @@ -186,5 +191,5 @@ void _liblinuxuplat_entry(int argc, char *argv[])
> >> /*
> >> * Enter Unikraft
> >> */
> >> - ukplat_entry(argc, argv);
> >> + ukplat_entry(&global_main_arg);
> >> }
> >>
> >
> > Thanks,
> >
> > Simon
> >
> > _______________________________________________
> > Minios-devel mailing list
> > Minios-devel@xxxxxxxxxxxxxxxxxxxx
> > https://lists.xenproject.org/mailman/listinfo/minios-devel
>
>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |