linux-kernel - RE: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 8 Mar 2018 04:22:40 +0000
From:   "Prakhya, Sai Praneeth" <sai.praneeth.prakhya@...el.com>
To:     Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
CC:     "linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "Lee@...r.kernel.org" <Lee@...r.kernel.org>,
        Chun-Yi <jlee@...e.com>, Borislav Petkov <bp@...en8.de>,
        "Luck, Tony" <tony.luck@...el.com>,
        Will Deacon <will.deacon@....com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        Mark Rutland <mark.rutland@....com>,
        Bhupesh Sharma <bhsharma@...hat.com>,
        "Neri, Ricardo" <ricardo.neri@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        Matt Fleming <matt@...eblueprint.co.uk>,
        "Zijlstra, Peter" <peter.zijlstra@...el.com>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        "Williams, Dan J" <dan.j.williams@...el.com>
Subject: RE: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some
 infrastructure to invoke all efi_runtime_services()

> > +struct workqueue_struct *efi_rts_wq;
> > +
> >  static bool disable_runtime;
> >  static int __init setup_noefi(char *arg)  { @@ -329,6 +331,19 @@
> > static int __init efisubsys_init(void)
> >                 return 0;
> >
> >         /*
> > +        * Since we process only one efi_runtime_service() at a time, an
> > +        * ordered workqueue (which creates only one execution context)
> > +        * should suffice all our needs.
> > +        */
> > +       efi_rts_wq = alloc_ordered_workqueue("efi_rts_workqueue", 0);
> 
> efi_rts_wq or efi_rts_workqueue?
> 
> > +       if (!efi_rts_wq) {
> > +               pr_err("Failed to create efi_rts_workqueue, EFI runtime services "
> 
> Same here.

Sure! I will make it consistent with "efi_rts_wq". Just tried to be more verbose
with names :)

[...]

> > +#define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5)                \
> > +({                                                                     \
> > +       struct efi_runtime_work efi_rts_work;                           \
> > +                                                                       \
> > +       INIT_WORK_ONSTACK(&efi_rts_work.work, efi_call_rts);            \
> > +       efi_rts_work.func = _rts;                                       \
> > +       efi_rts_work.arg1 = _arg1;                                      \
> > +       efi_rts_work.arg2 = _arg2;                                      \
> > +       efi_rts_work.arg3 = _arg3;                                      \
> > +       efi_rts_work.arg4 = _arg4;                                      \
> > +       efi_rts_work.arg5 = _arg5;                                      \
> > +       /*                                                              \
> > +        * queue_work() returns 0 if work was already on queue,         \
> > +        * _ideally_ this should never happen.                          \
> > +        */                                                             \
> > +       if (queue_work(efi_rts_wq, &efi_rts_work.work))                 \
> > +               flush_work(&efi_rts_work.work);                         \
> > +       else                                                            \
> > +               BUG();                                                  \
> 
> Thanks for the change! One remark, I would just do:

Sorry! but I am planning to remove BUG(). Looks like it could defeat the purpose
of patch. Please see Boris comments on the other thread.

[...]

> > +/*
> > + * efi_runtime_work:   Details of EFI Runtime Service work
> > + * @func:              EFI Runtime Service function identifier
> > + * @arg<1-5>:          EFI Runtime Service function arguments
> > + * @status:            Status of executing EFI Runtime Service
> > + */
> > +struct efi_runtime_work {
> > +       u8 func;
> > +       void *arg1;
> > +       void *arg2;
> > +       void *arg3;
> > +       void *arg4;
> > +       void *arg5;
> > +       efi_status_t status;
> > +       struct work_struct work;
> > +};
> 
> Why is efi_runtime_work in the .h at all?
> 

Thanks for the catch. I will move it to runtime-wrappers.c file and will make it
static too. It isn't being used in any other place.

> Please CC me for the next version! :-)

Sure! Sorry for that. I should have done in V2.

Regards,
Sai