The legacy hypercall handlers were originally added with
a comment explaining that "copying the argument structures in
HYPERVISOR_event_channel_op() and HYPERVISOR_physdev_op() into the local
variable is sufficiently safe" and only made sure to not write
past the end of the argument structure, the checks in linux/string.h
disagree with that, when link-time optimizations are used:
In function 'memcpy',
inlined from 'pirq_query_unmask' at drivers/xen/fallback.c:53:2,
inlined from '__startup_pirq' at drivers/xen/events/events_base.c:529:2,
inlined from 'restore_pirqs' at drivers/xen/events/events_base.c:1439:3,
inlined from 'xen_irq_resume' at drivers/xen/events/events_base.c:1581:2:
include/linux/string.h:350:3: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
__read_overflow2();
^
make[3]: *** [ccLujFNx.ltrans15.ltrans.o] Error 1
make[3]: Target 'all' not remade because of errors.
lto-wrapper: fatal error: make returned 2 exit status
compilation terminated.
ld: error: lto-wrapper failed
This changes the functions so that each argument is accessed with
exactly the correct length based on the command code.
Fixes: cf47a83fb06e ("xen/hypercall: fix hypercall fallback code for very old hypervisors")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
1 file changed, 53 insertions(+), 41 deletions(-)
diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
index b04fb64c5a91..eded8dd821ad 100644
--- a/drivers/xen/fallback.c
+++ b/drivers/xen/fallback.c
@@ -7,75 +7,87 @@
int xen_event_channel_op_compat(int cmd, void *arg)
{
- struct evtchn_op op;
+ struct evtchn_op op = { .cmd = cmd, };
+ size_t len;
int rc;
- op.cmd = cmd;
- memcpy(&op.u, arg, sizeof(op.u));
- rc = _hypercall1(int, event_channel_op_compat, &op);
-
switch (cmd) {
+ case EVTCHNOP_bind_interdomain:
+ len = sizeof(struct evtchn_bind_interdomain);
+ break;
+ case EVTCHNOP_bind_virq:
+ len = sizeof(struct evtchn_bind_virq);
+ break;
+ case EVTCHNOP_bind_pirq:
+ len = sizeof(struct evtchn_bind_pirq);
+ break;
case EVTCHNOP_close:
+ len = sizeof(struct evtchn_close);
+ break;
case EVTCHNOP_send:
+ len = sizeof(struct evtchn_send);
+ break;
+ case EVTCHNOP_alloc_unbound:
+ len = sizeof(struct evtchn_alloc_unbound);
+ break;
+ case EVTCHNOP_bind_ipi:
+ len = sizeof(struct evtchn_bind_ipi);
+ break;
+ case EVTCHNOP_status:
+ len = sizeof(struct evtchn_status);
+ break;
case EVTCHNOP_bind_vcpu:
+ len = sizeof(struct evtchn_bind_vcpu);
+ break;
case EVTCHNOP_unmask:
- /* no output */
+ len = sizeof(struct evtchn_unmask);
break;
-
-#define COPY_BACK(eop) \
- case EVTCHNOP_##eop: \
- memcpy(arg, &op.u.eop, sizeof(op.u.eop)); \
- break
-
- COPY_BACK(bind_interdomain);
- COPY_BACK(bind_virq);
- COPY_BACK(bind_pirq);
- COPY_BACK(status);
- COPY_BACK(alloc_unbound);
- COPY_BACK(bind_ipi);
-#undef COPY_BACK
-
default:
- WARN_ON(rc != -ENOSYS);
- break;
+ return -ENOSYS;
}
+ memcpy(&op.u, arg, len);
+ rc = _hypercall1(int, event_channel_op_compat, &op);
+ memcpy(arg, &op.u, len);
+
return rc;
}
EXPORT_SYMBOL_GPL(xen_event_channel_op_compat);
int xen_physdev_op_compat(int cmd, void *arg)
{
- struct physdev_op op;
+ struct physdev_op op = { .cmd = cmd, };
+ size_t len;
int rc;
- op.cmd = cmd;
- memcpy(&op.u, arg, sizeof(op.u));
- rc = _hypercall1(int, physdev_op_compat, &op);
-
switch (cmd) {
case PHYSDEVOP_IRQ_UNMASK_NOTIFY:
+ len = 0;
+ break;
+ case PHYSDEVOP_irq_status_query:
+ len = sizeof(struct physdev_irq_status_query);
+ break;
case PHYSDEVOP_set_iopl:
+ len = sizeof(struct physdev_set_iopl);
+ break;
case PHYSDEVOP_set_iobitmap:
+ len = sizeof(struct physdev_set_iobitmap);
+ break;
+ case PHYSDEVOP_apic_read:
case PHYSDEVOP_apic_write:
- /* no output */
+ len = sizeof(struct physdev_apic);
break;
-
-#define COPY_BACK(pop, fld) \
- case PHYSDEVOP_##pop: \
- memcpy(arg, &op.u.fld, sizeof(op.u.fld)); \
- break
-
- COPY_BACK(irq_status_query, irq_status_query);
- COPY_BACK(apic_read, apic_op);
- COPY_BACK(ASSIGN_VECTOR, irq_op);
-#undef COPY_BACK
-
- default:
- WARN_ON(rc != -ENOSYS);
+ case PHYSDEVOP_ASSIGN_VECTOR:
+ len = sizeof(struct physdev_irq);
break;
+ default:
+ return -ENOSYS;
}
+ memcpy(&op.u, arg, len);
+ rc = _hypercall1(int, physdev_op_compat, &op);
+ memcpy(arg, &op.u, len);
+
return rc;
}
EXPORT_SYMBOL_GPL(xen_physdev_op_compat);
--
2.9.0
On Fri, Feb 02, 2018 at 04:32:31PM +0100, Arnd Bergmann wrote:
> The legacy hypercall handlers were originally added with
> a comment explaining that "copying the argument structures in
> HYPERVISOR_event_channel_op() and HYPERVISOR_physdev_op() into the local
> variable is sufficiently safe" and only made sure to not write
> past the end of the argument structure, the checks in linux/string.h
> disagree with that, when link-time optimizations are used:
>
> In function 'memcpy',
> inlined from 'pirq_query_unmask' at drivers/xen/fallback.c:53:2,
> inlined from '__startup_pirq' at drivers/xen/events/events_base.c:529:2,
> inlined from 'restore_pirqs' at drivers/xen/events/events_base.c:1439:3,
> inlined from 'xen_irq_resume' at drivers/xen/events/events_base.c:1581:2:
> include/linux/string.h:350:3: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
> __read_overflow2();
> ^
> make[3]: *** [ccLujFNx.ltrans15.ltrans.o] Error 1
> make[3]: Target 'all' not remade because of errors.
> lto-wrapper: fatal error: make returned 2 exit status
> compilation terminated.
> ld: error: lto-wrapper failed
>
It was a more naive era. :P
> This changes the functions so that each argument is accessed with
> exactly the correct length based on the command code.
>
> Fixes: cf47a83fb06e ("xen/hypercall: fix hypercall fallback code for very old hypervisors")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
> index b04fb64c5a91..eded8dd821ad 100644
> --- a/drivers/xen/fallback.c
> +++ b/drivers/xen/fallback.c
> @@ -7,75 +7,87 @@
>
> int xen_event_channel_op_compat(int cmd, void *arg)
> {
> - struct evtchn_op op;
> + struct evtchn_op op = { .cmd = cmd, };
> + size_t len;
> int rc;
>
> - op.cmd = cmd;
> - memcpy(&op.u, arg, sizeof(op.u));
> - rc = _hypercall1(int, event_channel_op_compat, &op);
> -
> switch (cmd) {
> + case EVTCHNOP_bind_interdomain:
> + len = sizeof(struct evtchn_bind_interdomain);
> + break;
This was in the original code, but I'm slightly surpprised that we're
using a switch statement here instead of a table. I would have thought
this is a fast path but I don't know xen at all.
regards,
dan carpenter
On Fri, Feb 2, 2018 at 4:53 PM, Dan Carpenter <[email protected]> wrote:
> On Fri, Feb 02, 2018 at 04:32:31PM +0100, Arnd Bergmann wrote:
>> --- a/drivers/xen/fallback.c
>> +++ b/drivers/xen/fallback.c
>> @@ -7,75 +7,87 @@
>>
>> int xen_event_channel_op_compat(int cmd, void *arg)
>> {
>> - struct evtchn_op op;
>> + struct evtchn_op op = { .cmd = cmd, };
>> + size_t len;
>> int rc;
>>
>> - op.cmd = cmd;
>> - memcpy(&op.u, arg, sizeof(op.u));
>> - rc = _hypercall1(int, event_channel_op_compat, &op);
>> -
>> switch (cmd) {
>> + case EVTCHNOP_bind_interdomain:
>> + len = sizeof(struct evtchn_bind_interdomain);
>> + break;
>
> This was in the original code, but I'm slightly surpprised that we're
> using a switch statement here instead of a table. I would have thought
> this is a fast path but I don't know xen at all.
I thought about using a table, but figured the switch statement
had a lower risk of getting something slightly wrong during the
conversion.
I would expect gcc to turn this into a table lookup, since all the
constants are consecutive, but it should not really matter since
this is only the fallback path for ancient Xen releases. When Xen
guest support was first merged in 2007, it was already
deprecated.
Arnd
On Fri, Feb 02, 2018 at 05:11:02PM +0100, Arnd Bergmann wrote:
> On Fri, Feb 2, 2018 at 4:53 PM, Dan Carpenter <[email protected]> wrote:
> > On Fri, Feb 02, 2018 at 04:32:31PM +0100, Arnd Bergmann wrote:
> >> switch (cmd) {
> >> + case EVTCHNOP_bind_interdomain:
> >> + len = sizeof(struct evtchn_bind_interdomain);
> >> + break;
> >
> > This was in the original code, but I'm slightly surpprised that we're
> > using a switch statement here instead of a table. I would have thought
> > this is a fast path but I don't know xen at all.
>
> I thought about using a table, but figured the switch statement
> had a lower risk of getting something slightly wrong during the
> conversion.
>
> I would expect gcc to turn this into a table lookup, since all the
> constants are consecutive, but it should not really matter since
> this is only the fallback path for ancient Xen releases. When Xen
> guest support was first merged in 2007, it was already
> deprecated.
>
Ah. Ok. That makes sense.
regards,
dan carpenter
On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
> The legacy hypercall handlers were originally added with
> a comment explaining that "copying the argument structures in
> HYPERVISOR_event_channel_op() and HYPERVISOR_physdev_op() into the local
> variable is sufficiently safe" and only made sure to not write
> past the end of the argument structure, the checks in linux/string.h
> disagree with that, when link-time optimizations are used:
>
> In function 'memcpy',
> inlined from 'pirq_query_unmask' at drivers/xen/fallback.c:53:2,
> inlined from '__startup_pirq' at drivers/xen/events/events_base.c:529:2,
> inlined from 'restore_pirqs' at drivers/xen/events/events_base.c:1439:3,
> inlined from 'xen_irq_resume' at drivers/xen/events/events_base.c:1581:2:
> include/linux/string.h:350:3: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
> __read_overflow2();
> ^
> make[3]: *** [ccLujFNx.ltrans15.ltrans.o] Error 1
> make[3]: Target 'all' not remade because of errors.
> lto-wrapper: fatal error: make returned 2 exit status
> compilation terminated.
> ld: error: lto-wrapper failed
>
> This changes the functions so that each argument is accessed with
> exactly the correct length based on the command code.
>
> Fixes: cf47a83fb06e ("xen/hypercall: fix hypercall fallback code for very old hypervisors")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
> index b04fb64c5a91..eded8dd821ad 100644
> --- a/drivers/xen/fallback.c
> +++ b/drivers/xen/fallback.c
> @@ -7,75 +7,87 @@
>
> int xen_event_channel_op_compat(int cmd, void *arg)
> {
> - struct evtchn_op op;
> + struct evtchn_op op = { .cmd = cmd, };
> + size_t len;
> int rc;
>
> - op.cmd = cmd;
> - memcpy(&op.u, arg, sizeof(op.u));
> - rc = _hypercall1(int, event_channel_op_compat, &op);
> -
> switch (cmd) {
> + case EVTCHNOP_bind_interdomain:
> + len = sizeof(struct evtchn_bind_interdomain);
> + break;
> + case EVTCHNOP_bind_virq:
> + len = sizeof(struct evtchn_bind_virq);
> + break;
> + case EVTCHNOP_bind_pirq:
> + len = sizeof(struct evtchn_bind_pirq);
> + break;
> case EVTCHNOP_close:
> + len = sizeof(struct evtchn_close);
> + break;
> case EVTCHNOP_send:
> + len = sizeof(struct evtchn_send);
> + break;
> + case EVTCHNOP_alloc_unbound:
> + len = sizeof(struct evtchn_alloc_unbound);
> + break;
> + case EVTCHNOP_bind_ipi:
> + len = sizeof(struct evtchn_bind_ipi);
> + break;
> + case EVTCHNOP_status:
> + len = sizeof(struct evtchn_status);
> + break;
> case EVTCHNOP_bind_vcpu:
> + len = sizeof(struct evtchn_bind_vcpu);
> + break;
> case EVTCHNOP_unmask:
> - /* no output */
> + len = sizeof(struct evtchn_unmask);
> break;
> -
> -#define COPY_BACK(eop) \
> - case EVTCHNOP_##eop: \
> - memcpy(arg, &op.u.eop, sizeof(op.u.eop)); \
> - break
> -
> - COPY_BACK(bind_interdomain);
> - COPY_BACK(bind_virq);
> - COPY_BACK(bind_pirq);
> - COPY_BACK(status);
> - COPY_BACK(alloc_unbound);
> - COPY_BACK(bind_ipi);
> -#undef COPY_BACK
> -
> default:
> - WARN_ON(rc != -ENOSYS);
> - break;
> + return -ENOSYS;
> }
>
> + memcpy(&op.u, arg, len);
> + rc = _hypercall1(int, event_channel_op_compat, &op);
> + memcpy(arg, &op.u, len);
We don't copy back for all commands, only those that are COPY_BACK.
> +
> return rc;
> }
> EXPORT_SYMBOL_GPL(xen_event_channel_op_compat);
>
> int xen_physdev_op_compat(int cmd, void *arg)
> {
> - struct physdev_op op;
> + struct physdev_op op = { .cmd = cmd, };
> + size_t len;
> int rc;
>
> - op.cmd = cmd;
> - memcpy(&op.u, arg, sizeof(op.u));
> - rc = _hypercall1(int, physdev_op_compat, &op);
> -
> switch (cmd) {
> case PHYSDEVOP_IRQ_UNMASK_NOTIFY:
> + len = 0;
> + break;
> + case PHYSDEVOP_irq_status_query:
> + len = sizeof(struct physdev_irq_status_query);
> + break;
> case PHYSDEVOP_set_iopl:
> + len = sizeof(struct physdev_set_iopl);
> + break;
> case PHYSDEVOP_set_iobitmap:
> + len = sizeof(struct physdev_set_iobitmap);
> + break;
> + case PHYSDEVOP_apic_read:
> case PHYSDEVOP_apic_write:
> - /* no output */
> + len = sizeof(struct physdev_apic);
> break;
> -
> -#define COPY_BACK(pop, fld) \
> - case PHYSDEVOP_##pop: \
> - memcpy(arg, &op.u.fld, sizeof(op.u.fld)); \
> - break
> -
> - COPY_BACK(irq_status_query, irq_status_query);
> - COPY_BACK(apic_read, apic_op);
> - COPY_BACK(ASSIGN_VECTOR, irq_op);
> -#undef COPY_BACK
> -
> - default:
> - WARN_ON(rc != -ENOSYS);
> + case PHYSDEVOP_ASSIGN_VECTOR:
> + len = sizeof(struct physdev_irq);
> break;
> + default:
> + return -ENOSYS;
> }
>
> + memcpy(&op.u, arg, len);
> + rc = _hypercall1(int, physdev_op_compat, &op);
> + memcpy(arg, &op.u, len);
And the same is true here.
-boris
> +
> return rc;
> }
> EXPORT_SYMBOL_GPL(xen_physdev_op_compat);
On Sat, Feb 3, 2018 at 12:33 AM, Boris Ostrovsky
<[email protected]> wrote:
> On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
>> The legacy hypercall handlers were originally added with
>> a comment explaining that "copying the argument structures in
>> HYPERVISOR_event_channel_op() and HYPERVISOR_physdev_op() into the local
>> variable is sufficiently safe" and only made sure to not write
>> past the end of the argument structure, the checks in linux/string.h
>> disagree with that, when link-time optimizations are used:
>>
>> In function 'memcpy',
>> inlined from 'pirq_query_unmask' at drivers/xen/fallback.c:53:2,
>> inlined from '__startup_pirq' at drivers/xen/events/events_base.c:529:2,
>> inlined from 'restore_pirqs' at drivers/xen/events/events_base.c:1439:3,
>> inlined from 'xen_irq_resume' at drivers/xen/events/events_base.c:1581:2:
>> include/linux/string.h:350:3: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
>> __read_overflow2();
>> ^
>> make[3]: *** [ccLujFNx.ltrans15.ltrans.o] Error 1
>> make[3]: Target 'all' not remade because of errors.
>> lto-wrapper: fatal error: make returned 2 exit status
>> compilation terminated.
>> ld: error: lto-wrapper failed
>>
>> This changes the functions so that each argument is accessed with
>> exactly the correct length based on the command code.
>>
>> Fixes: cf47a83fb06e ("xen/hypercall: fix hypercall fallback code for very old hypervisors")
>> Signed-off-by: Arnd Bergmann <[email protected]>
>> ---
>> drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
>> 1 file changed, 53 insertions(+), 41 deletions(-)
>>
>> default:
>> - WARN_ON(rc != -ENOSYS);
>> - break;
>> + return -ENOSYS;
>> }
>>
>> + memcpy(&op.u, arg, len);
>> + rc = _hypercall1(int, event_channel_op_compat, &op);
>> + memcpy(arg, &op.u, len);
>
>
> We don't copy back for all commands, only those that are COPY_BACK.
Not sure what you mean. Is it harmful to copy back the data for the others
in any way? Otherwise I wouldn't micro-optimize this.
Arnd
On 02/03/2018 10:12 AM, Arnd Bergmann wrote:
> On Sat, Feb 3, 2018 at 12:33 AM, Boris Ostrovsky
> <[email protected]> wrote:
>> On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
>>> The legacy hypercall handlers were originally added with
>>> a comment explaining that "copying the argument structures in
>>> HYPERVISOR_event_channel_op() and HYPERVISOR_physdev_op() into the local
>>> variable is sufficiently safe" and only made sure to not write
>>> past the end of the argument structure, the checks in linux/string.h
>>> disagree with that, when link-time optimizations are used:
>>>
>>> In function 'memcpy',
>>> inlined from 'pirq_query_unmask' at drivers/xen/fallback.c:53:2,
>>> inlined from '__startup_pirq' at drivers/xen/events/events_base.c:529:2,
>>> inlined from 'restore_pirqs' at drivers/xen/events/events_base.c:1439:3,
>>> inlined from 'xen_irq_resume' at drivers/xen/events/events_base.c:1581:2:
>>> include/linux/string.h:350:3: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
>>> __read_overflow2();
>>> ^
>>> make[3]: *** [ccLujFNx.ltrans15.ltrans.o] Error 1
>>> make[3]: Target 'all' not remade because of errors.
>>> lto-wrapper: fatal error: make returned 2 exit status
>>> compilation terminated.
>>> ld: error: lto-wrapper failed
>>>
>>> This changes the functions so that each argument is accessed with
>>> exactly the correct length based on the command code.
>>>
>>> Fixes: cf47a83fb06e ("xen/hypercall: fix hypercall fallback code for very old hypervisors")
>>> Signed-off-by: Arnd Bergmann <[email protected]>
>>> ---
>>> drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
>>> 1 file changed, 53 insertions(+), 41 deletions(-)
>>>
>
>>> default:
>>> - WARN_ON(rc != -ENOSYS);
>>> - break;
>>> + return -ENOSYS;
>>> }
>>>
>>> + memcpy(&op.u, arg, len);
>>> + rc = _hypercall1(int, event_channel_op_compat, &op);
>>> + memcpy(arg, &op.u, len);
>>
>>
>> We don't copy back for all commands, only those that are COPY_BACK.
>
> Not sure what you mean. Is it harmful to copy back the data for the others
> in any way? Otherwise I wouldn't micro-optimize this.
I should have checked the original commit for fallback.c --- the code
that it replaced was doing copybacks for all hypercalls and selective
copybacks is an optimization introduced in that commit.
-boris
On Sat, Feb 3, 2018 at 6:08 PM, Boris Ostrovsky
<[email protected]> wrote:
>
>
> On 02/03/2018 10:12 AM, Arnd Bergmann wrote:
>>
>> On Sat, Feb 3, 2018 at 12:33 AM, Boris Ostrovsky
>> <[email protected]> wrote:
>>>
>>> On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
>>>>
>>>> The legacy hypercall handlers were originally added with
>>>> a comment explaining that "copying the argument structures in
>>>> HYPERVISOR_event_channel_op() and HYPERVISOR_physdev_op() into the local
>>>> variable is sufficiently safe" and only made sure to not write
>>>> past the end of the argument structure, the checks in linux/string.h
>>>> disagree with that, when link-time optimizations are used:
>>>>
>>>> In function 'memcpy',
>>>> inlined from 'pirq_query_unmask' at drivers/xen/fallback.c:53:2,
>>>> inlined from '__startup_pirq' at
>>>> drivers/xen/events/events_base.c:529:2,
>>>> inlined from 'restore_pirqs' at
>>>> drivers/xen/events/events_base.c:1439:3,
>>>> inlined from 'xen_irq_resume' at
>>>> drivers/xen/events/events_base.c:1581:2:
>>>> include/linux/string.h:350:3: error: call to '__read_overflow2' declared
>>>> with attribute error: detected read beyond size of object passed as 2nd
>>>> parameter
>>>> __read_overflow2();
>>>> ^
>>>> make[3]: *** [ccLujFNx.ltrans15.ltrans.o] Error 1
>>>> make[3]: Target 'all' not remade because of errors.
>>>> lto-wrapper: fatal error: make returned 2 exit status
>>>> compilation terminated.
>>>> ld: error: lto-wrapper failed
>>>>
>>>> This changes the functions so that each argument is accessed with
>>>> exactly the correct length based on the command code.
>>>>
>>>> Fixes: cf47a83fb06e ("xen/hypercall: fix hypercall fallback code for
>>>> very old hypervisors")
>>>> Signed-off-by: Arnd Bergmann <[email protected]>
>>>> ---
>>>> drivers/xen/fallback.c | 94
>>>> ++++++++++++++++++++++++++++----------------------
>>>> 1 file changed, 53 insertions(+), 41 deletions(-)
>>>>
>>
>>>> default:
>>>> - WARN_ON(rc != -ENOSYS);
>>>> - break;
>>>> + return -ENOSYS;
>>>> }
>>>>
>>>> + memcpy(&op.u, arg, len);
>>>> + rc = _hypercall1(int, event_channel_op_compat, &op);
>>>> + memcpy(arg, &op.u, len);
>>>
>>>
>>>
>>> We don't copy back for all commands, only those that are COPY_BACK.
>>
>>
>> Not sure what you mean. Is it harmful to copy back the data for the others
>> in any way? Otherwise I wouldn't micro-optimize this.
>
>
>
> I should have checked the original commit for fallback.c --- the code that
> it replaced was doing copybacks for all hypercalls and selective copybacks
> is an optimization introduced in that commit.
It was not an optimization but a correctness fix to avoid overflowing
the caller stack on the copy-back operation. What I tried to explain
in my commit message is that the same fix is also needed on
the copy-out before it. It's only a read access beyond the end
of a local variable, but not both the static checks and kasan-stack
get alarmed about it.
Arnd
On 02/04/2018 10:35 AM, Arnd Bergmann wrote:
> On Sat, Feb 3, 2018 at 6:08 PM, Boris Ostrovsky
> <[email protected]> wrote:
>>
>> On 02/03/2018 10:12 AM, Arnd Bergmann wrote:
>>> On Sat, Feb 3, 2018 at 12:33 AM, Boris Ostrovsky
>>> <[email protected]> wrote:
>>>> On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
>>>>> The legacy hypercall handlers were originally added with
>>>>> a comment explaining that "copying the argument structures in
>>>>> HYPERVISOR_event_channel_op() and HYPERVISOR_physdev_op() into the local
>>>>> variable is sufficiently safe" and only made sure to not write
>>>>> past the end of the argument structure, the checks in linux/string.h
>>>>> disagree with that, when link-time optimizations are used:
>>>>>
>>>>> In function 'memcpy',
>>>>> inlined from 'pirq_query_unmask' at drivers/xen/fallback.c:53:2,
>>>>> inlined from '__startup_pirq' at
>>>>> drivers/xen/events/events_base.c:529:2,
>>>>> inlined from 'restore_pirqs' at
>>>>> drivers/xen/events/events_base.c:1439:3,
>>>>> inlined from 'xen_irq_resume' at
>>>>> drivers/xen/events/events_base.c:1581:2:
>>>>> include/linux/string.h:350:3: error: call to '__read_overflow2' declared
>>>>> with attribute error: detected read beyond size of object passed as 2nd
>>>>> parameter
>>>>> __read_overflow2();
>>>>> ^
>>>>> make[3]: *** [ccLujFNx.ltrans15.ltrans.o] Error 1
>>>>> make[3]: Target 'all' not remade because of errors.
>>>>> lto-wrapper: fatal error: make returned 2 exit status
>>>>> compilation terminated.
>>>>> ld: error: lto-wrapper failed
>>>>>
>>>>> This changes the functions so that each argument is accessed with
>>>>> exactly the correct length based on the command code.
>>>>>
>>>>> Fixes: cf47a83fb06e ("xen/hypercall: fix hypercall fallback code for
>>>>> very old hypervisors")
>>>>> Signed-off-by: Arnd Bergmann <[email protected]>
>>>>> ---
>>>>> drivers/xen/fallback.c | 94
>>>>> ++++++++++++++++++++++++++++----------------------
>>>>> 1 file changed, 53 insertions(+), 41 deletions(-)
>>>>>
>>>>> default:
>>>>> - WARN_ON(rc != -ENOSYS);
>>>>> - break;
>>>>> + return -ENOSYS;
>>>>> }
>>>>>
>>>>> + memcpy(&op.u, arg, len);
>>>>> + rc = _hypercall1(int, event_channel_op_compat, &op);
>>>>> + memcpy(arg, &op.u, len);
>>>>
>>>>
>>>> We don't copy back for all commands, only those that are COPY_BACK.
>>>
>>> Not sure what you mean. Is it harmful to copy back the data for the others
>>> in any way? Otherwise I wouldn't micro-optimize this.
>>
>>
>> I should have checked the original commit for fallback.c --- the code that
>> it replaced was doing copybacks for all hypercalls and selective copybacks
>> is an optimization introduced in that commit.
> It was not an optimization but a correctness fix to avoid overflowing
> the caller stack on the copy-back operation. What I tried to explain
> in my commit message is that the same fix is also needed on
> the copy-out before it. It's only a read access beyond the end
> of a local variable, but not both the static checks and kasan-stack
> get alarmed about it.
>
Yes, I understand that. I was referring to:
Move the fallback code into out-of-line functions, and handle all of
the operations known by this old a hypervisor individually: *Some don't
require copying back anything at all*, and for the rest use the
individual argument structures' sizes rather than the container's
in the original commit. I.e. prior to that commit we *were* copying back
for all commands (although possibly with potentially incorrect size). So
not copying back for some commands was an optimization.
In any case,
Reviewed-by: Boris Ostrovsky <[email protected]>
From: Boris Ostrovsky
> Sent: 02 February 2018 23:34
...
> > switch (cmd) {
> > + case EVTCHNOP_bind_interdomain:
> > + len = sizeof(struct evtchn_bind_interdomain);
> > + break;
> > + case EVTCHNOP_bind_virq:
> > + len = sizeof(struct evtchn_bind_virq);
> > + break;
> > + case EVTCHNOP_bind_pirq:
> > + len = sizeof(struct evtchn_bind_pirq);
> > + break;
> > case EVTCHNOP_close:
> > + len = sizeof(struct evtchn_close);
> > + break;
> > case EVTCHNOP_send:
> > + len = sizeof(struct evtchn_send);
> > + break;
> > + case EVTCHNOP_alloc_unbound:
> > + len = sizeof(struct evtchn_alloc_unbound);
> > + break;
> > + case EVTCHNOP_bind_ipi:
> > + len = sizeof(struct evtchn_bind_ipi);
> > + break;
> > + case EVTCHNOP_status:
> > + len = sizeof(struct evtchn_status);
> > + break;
> > case EVTCHNOP_bind_vcpu:
> > + len = sizeof(struct evtchn_bind_vcpu);
> > + break;
> > case EVTCHNOP_unmask:
> > - /* no output */
> > + len = sizeof(struct evtchn_unmask);
> > break;
Are the EVTCHNOP_xxx values dense?
In which case an array is almost certainly better than the switch statement.
David
On Mon, Feb 5, 2018 at 1:11 PM, David Laight <[email protected]> wrote:
> From: Boris Ostrovsky
>> Sent: 02 February 2018 23:34
> ...
>> > switch (cmd) {
>> > + case EVTCHNOP_bind_interdomain:
>> > + len = sizeof(struct evtchn_bind_interdomain);
>> > + break;
>> > + case EVTCHNOP_bind_virq:
>> > + len = sizeof(struct evtchn_bind_virq);
>> > + break;
>> > + case EVTCHNOP_bind_pirq:
>> > + len = sizeof(struct evtchn_bind_pirq);
>> > + break;
>> > case EVTCHNOP_close:
>> > + len = sizeof(struct evtchn_close);
>> > + break;
>> > case EVTCHNOP_send:
>> > + len = sizeof(struct evtchn_send);
>> > + break;
>> > + case EVTCHNOP_alloc_unbound:
>> > + len = sizeof(struct evtchn_alloc_unbound);
>> > + break;
>> > + case EVTCHNOP_bind_ipi:
>> > + len = sizeof(struct evtchn_bind_ipi);
>> > + break;
>> > + case EVTCHNOP_status:
>> > + len = sizeof(struct evtchn_status);
>> > + break;
>> > case EVTCHNOP_bind_vcpu:
>> > + len = sizeof(struct evtchn_bind_vcpu);
>> > + break;
>> > case EVTCHNOP_unmask:
>> > - /* no output */
>> > + len = sizeof(struct evtchn_unmask);
>> > break;
>
> Are the EVTCHNOP_xxx values dense?
> In which case an array is almost certainly better than the switch statement.
They are, yes. PHYSDEVOP_xxx are also consecutive by start at '4'.
Dan made the same comment earlier, and I replied that my I had
considered it but went for the more failsafe route. I also verified my
assumption now that gcc in fact is smart enough to turn this
into a table by itself:
xen_physdev_op_compat:
pushq %r13 #
leal -4(%rdi), %eax #, _2
pushq %r12 #
pushq %rbp #
pushq %rbx #
subq $24, %rsp #,
# /git/arm-soc/drivers/xen/fallback.c:59: struct physdev_op op =
{ .cmd = cmd, };
movq $0, 4(%rsp) #, MEM[(struct physdev_op *)&op + 4B]
movq $0, 12(%rsp) #, MEM[(struct physdev_op *)&op + 4B]
movl $0, 20(%rsp) #, MEM[(struct physdev_op *)&op + 4B]
movl %edi, (%rsp) # cmd, op.cmd
cmpl $6, %eax #, _2
ja .L8 #,
movl %eax, %edi # _2, _2
# /git/arm-soc/drivers/xen/fallback.c:87: memcpy(&op.u, arg, len);
leaq 8(%rsp), %r12 #, tmp98
movq %rsi, %rbx # arg, arg
movq CSWTCH.17(,%rdi,8), %r13 # CSWTCH.17, _5
movq %r12, %rdi # tmp98,
movq %r13, %rdx # _5,
call __memcpy #
# /git/arm-soc/drivers/xen/fallback.c:88: rc = _hypercall1(int,
physdev_op_compat, &op);
movq %rsp, %rdi #, __arg1
#APP
# 88 "/git/arm-soc/drivers/xen/fallback.c" 1
call hypercall_page+608 #
Arnd
From: Arnd Bergmann
> Sent: 05 February 2018 12:37
....
> > Are the EVTCHNOP_xxx values dense?
> > In which case an array is almost certainly better than the switch statement.
>
> They are, yes. PHYSDEVOP_xxx are also consecutive by start at '4'.
> Dan made the same comment earlier, and I replied that my I had
> considered it but went for the more failsafe route. I also verified my
> assumption now that gcc in fact is smart enough to turn this
> into a table by itself:
I've never spotted that optimisation, must be fairly new.
David
On Mon, Feb 5, 2018 at 2:58 PM, David Laight <[email protected]> wrote:
> From: Arnd Bergmann
>> Sent: 05 February 2018 12:37
> ....
>> > Are the EVTCHNOP_xxx values dense?
>> > In which case an array is almost certainly better than the switch statement.
>>
>> They are, yes. PHYSDEVOP_xxx are also consecutive by start at '4'.
>> Dan made the same comment earlier, and I replied that my I had
>> considered it but went for the more failsafe route. I also verified my
>> assumption now that gcc in fact is smart enough to turn this
>> into a table by itself:
>
> I've never spotted that optimisation, must be fairly new.
Indeed, I checked again and found that most compilers have a
less efficient jump table based version, the output I posted was
from gcc-8, this is what I get with gcc-4.8 through gcc-7:
xen_event_channel_op_compat:
pushq %r13 #
pushq %r12 #
pushq %rbp #
pushq %rbx #
movl $-38, %ebx #, <retval>
subq $32, %rsp #,
# /git/arm-soc/drivers/xen/fallback.c:14: switch (cmd) {
cmpl $9, %edi #, cmd
# /git/arm-soc/drivers/xen/fallback.c:10: struct evtchn_op op =
{ .cmd = cmd, };
movq $0, 8(%rsp) #, MEM[(struct evtchn_op *)&op + 4B]
movq $0, 16(%rsp) #, MEM[(struct evtchn_op *)&op + 4B]
movq $0, 24(%rsp) #, MEM[(struct evtchn_op *)&op + 4B]
movl %edi, 4(%rsp) # cmd, op.cmd
# /git/arm-soc/drivers/xen/fallback.c:14: switch (cmd) {
ja .L1 #,
movl %edi, %eax # cmd, cmd
jmp *.L4(,%rax,8) #
.section .rodata
.align 8
.align 4
.L4:
.quad .L9
.quad .L9
.quad .L9
.quad .L5
.quad .L5
.quad .L6
.quad .L7
.quad .L7
.quad .L7
.quad .L5
.text
.L7:
# /git/arm-soc/drivers/xen/fallback.c:31: len =
sizeof(struct evtchn_alloc_unbound);
movl $8, %r12d #, len
.L3:
# /git/arm-soc/drivers/xen/fallback.c:49: memcpy(&op.u, arg, len);
leaq 8(%rsp), %r13 #, tmp98
movq %r12, %rdx # len,
movq %rsi, %rbp # arg, arg
movq %r13, %rdi # tmp98,
call __memcpy #
# /git/arm-soc/drivers/xen/fallback.c:50: rc = _hypercall1(int,
event_channel_op_compat, &op);
leaq 4(%rsp), %rdi #, tmp104
#APP
# 50 "/git/arm-soc/drivers/xen/fallback.c" 1
call hypercall_page+512 #
# 0 "" 2
# /git/arm-soc/drivers/xen/fallback.c:51: memcpy(arg, &op.u, len);
#NO_APP
movq %r12, %rdx # len,
movq %r13, %rsi # tmp98,
movq %rbp, %rdi # arg,
# /git/arm-soc/drivers/xen/fallback.c:50: rc = _hypercall1(int,
event_channel_op_compat, &op);
movl %eax, %ebx # __res.7_3, <retval>
# /git/arm-soc/drivers/xen/fallback.c:51: memcpy(arg, &op.u, len);
call __memcpy #
.L1:
# /git/arm-soc/drivers/xen/fallback.c:54: }
addq $32, %rsp #,
movl %ebx, %eax # <retval>,
popq %rbx #
popq %rbp #
popq %r12 #
popq %r13 #
ret
.L5:
# /git/arm-soc/drivers/xen/fallback.c:25: len =
sizeof(struct evtchn_close);
movl $4, %r12d #, len
jmp .L3 #
.L9:
# /git/arm-soc/drivers/xen/fallback.c:16: len =
sizeof(struct evtchn_bind_interdomain);
movl $12, %r12d #, len
jmp .L3 #
.L6:
# /git/arm-soc/drivers/xen/fallback.c:37: len =
sizeof(struct evtchn_status);
movl $24, %r12d #, len
# /git/arm-soc/drivers/xen/fallback.c:38: break;
jmp .L3 #
.size xen_event_channel_op_compat, .-xen_event_channel_op_compat
.p2align 4,,15
which isn't all that bad, but gets slightly worse when you compile with
-mindirect-branch=thunk-extern, the total size now grows from 474
bytes with gcc-8 to 525 bytes with gcc-7+retpoline.
Arnd