[v2,6/7] flask: drop dead compat translation code
diff mbox series

Message ID 7711f68d-394e-a74f-81fa-51f8447174ce@suse.com
State Superseded
Headers show
Series
  • x86: compat header generation and checking adjustments
Related show

Commit Message

Jan Beulich July 1, 2020, 10:28 a.m. UTC
Translation macros aren't needed at all (or else a devicetree_label
entry would have been missing), and userlist has been removed quite some
time ago.

No functional change.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Roger Pau Monne July 14, 2020, 2:58 p.m. UTC | #1
On Wed, Jul 01, 2020 at 12:28:07PM +0200, Jan Beulich wrote:
> Translation macros aren't needed at all (or else a devicetree_label
> entry would have been missing), and userlist has been removed quite some
> time ago.
> 
> No functional change.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -148,14 +148,11 @@
>  ?	xenoprof_init			xenoprof.h
>  ?	xenoprof_passive		xenoprof.h
>  ?	flask_access			xsm/flask_op.h
> -!	flask_boolean			xsm/flask_op.h
>  ?	flask_cache_stats		xsm/flask_op.h
>  ?	flask_hash_stats		xsm/flask_op.h
> -!	flask_load			xsm/flask_op.h
>  ?	flask_ocontext			xsm/flask_op.h
>  ?	flask_peersid			xsm/flask_op.h
>  ?	flask_relabel			xsm/flask_op.h
>  ?	flask_setavc_threshold		xsm/flask_op.h
>  ?	flask_setenforce		xsm/flask_op.h
> -!	flask_sid_context		xsm/flask_op.h
>  ?	flask_transition		xsm/flask_op.h

Shouldn't those become checks then?

Sorry I realize my knowledge of all this compat stuff is very poor.

Thanks.
Jan Beulich July 15, 2020, 6:42 a.m. UTC | #2
On 14.07.2020 16:58, Roger Pau Monné wrote:
> On Wed, Jul 01, 2020 at 12:28:07PM +0200, Jan Beulich wrote:
>> Translation macros aren't needed at all (or else a devicetree_label
>> entry would have been missing), and userlist has been removed quite some
>> time ago.
>>
>> No functional change.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/include/xlat.lst
>> +++ b/xen/include/xlat.lst
>> @@ -148,14 +148,11 @@
>>  ?	xenoprof_init			xenoprof.h
>>  ?	xenoprof_passive		xenoprof.h
>>  ?	flask_access			xsm/flask_op.h
>> -!	flask_boolean			xsm/flask_op.h
>>  ?	flask_cache_stats		xsm/flask_op.h
>>  ?	flask_hash_stats		xsm/flask_op.h
>> -!	flask_load			xsm/flask_op.h
>>  ?	flask_ocontext			xsm/flask_op.h
>>  ?	flask_peersid			xsm/flask_op.h
>>  ?	flask_relabel			xsm/flask_op.h
>>  ?	flask_setavc_threshold		xsm/flask_op.h
>>  ?	flask_setenforce		xsm/flask_op.h
>> -!	flask_sid_context		xsm/flask_op.h
>>  ?	flask_transition		xsm/flask_op.h
> 
> Shouldn't those become checks then?

No, checking will never succeed for structures containing
XEN_GUEST_HANDLE(). But there's no point in generating xlat macros
when they're never used. There are two fundamentally different
strategies for handling the compat hypercalls: One is to wrap a
translation layer around the native hypercall. That's where the
xlat macros come into play. The other, used here, is to compile
the entire hypercall function a second time, arranging for the
compat structures to get used in place of the native ones. There
are no xlat macros involved here, all that's needed are correctly
translated structures. (For completeness, x86's MCA hypercall
uses yet another, quite adhoc strategy for handling, but also not
involving any xlat macro use. Hence the consideration there to
possibly drop the respective lines from the file here.)

Jan
Roger Pau Monne July 15, 2020, 8:41 a.m. UTC | #3
On Wed, Jul 15, 2020 at 08:42:44AM +0200, Jan Beulich wrote:
> On 14.07.2020 16:58, Roger Pau Monné wrote:
> > On Wed, Jul 01, 2020 at 12:28:07PM +0200, Jan Beulich wrote:
> >> Translation macros aren't needed at all (or else a devicetree_label
> >> entry would have been missing), and userlist has been removed quite some
> >> time ago.
> >>
> >> No functional change.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/xen/include/xlat.lst
> >> +++ b/xen/include/xlat.lst
> >> @@ -148,14 +148,11 @@
> >>  ?	xenoprof_init			xenoprof.h
> >>  ?	xenoprof_passive		xenoprof.h
> >>  ?	flask_access			xsm/flask_op.h
> >> -!	flask_boolean			xsm/flask_op.h
> >>  ?	flask_cache_stats		xsm/flask_op.h
> >>  ?	flask_hash_stats		xsm/flask_op.h
> >> -!	flask_load			xsm/flask_op.h
> >>  ?	flask_ocontext			xsm/flask_op.h
> >>  ?	flask_peersid			xsm/flask_op.h
> >>  ?	flask_relabel			xsm/flask_op.h
> >>  ?	flask_setavc_threshold		xsm/flask_op.h
> >>  ?	flask_setenforce		xsm/flask_op.h
> >> -!	flask_sid_context		xsm/flask_op.h
> >>  ?	flask_transition		xsm/flask_op.h
> > 
> > Shouldn't those become checks then?
> 
> No, checking will never succeed for structures containing
> XEN_GUEST_HANDLE(). But there's no point in generating xlat macros
> when they're never used. There are two fundamentally different
> strategies for handling the compat hypercalls: One is to wrap a
> translation layer around the native hypercall. That's where the
> xlat macros come into play. The other, used here, is to compile
> the entire hypercall function a second time, arranging for the
> compat structures to get used in place of the native ones. There
> are no xlat macros involved here, all that's needed are correctly
> translated structures. (For completeness, x86's MCA hypercall
> uses yet another, quite adhoc strategy for handling, but also not
> involving any xlat macro use. Hence the consideration there to
> possibly drop the respective lines from the file here.)

Thanks, I think this explanation is helpful and I wonder whether it
would be possible to have something along this lines in a file or as a
comment somewhere, maybe at the top of xlat.lst?

Also could you add a line to the commit message noting that flask code
doesn't use any of the translation macros because it follows a
different approach to compat handling?

IMO the compat code is complicated to understand, and it also seems to
be mostly undocumented.

For the patch:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.
Jan Beulich July 15, 2020, 8:52 a.m. UTC | #4
On 15.07.2020 10:41, Roger Pau Monné wrote:
> On Wed, Jul 15, 2020 at 08:42:44AM +0200, Jan Beulich wrote:
>> On 14.07.2020 16:58, Roger Pau Monné wrote:
>>> On Wed, Jul 01, 2020 at 12:28:07PM +0200, Jan Beulich wrote:
>>>> Translation macros aren't needed at all (or else a devicetree_label
>>>> entry would have been missing), and userlist has been removed quite some
>>>> time ago.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/xen/include/xlat.lst
>>>> +++ b/xen/include/xlat.lst
>>>> @@ -148,14 +148,11 @@
>>>>  ?	xenoprof_init			xenoprof.h
>>>>  ?	xenoprof_passive		xenoprof.h
>>>>  ?	flask_access			xsm/flask_op.h
>>>> -!	flask_boolean			xsm/flask_op.h
>>>>  ?	flask_cache_stats		xsm/flask_op.h
>>>>  ?	flask_hash_stats		xsm/flask_op.h
>>>> -!	flask_load			xsm/flask_op.h
>>>>  ?	flask_ocontext			xsm/flask_op.h
>>>>  ?	flask_peersid			xsm/flask_op.h
>>>>  ?	flask_relabel			xsm/flask_op.h
>>>>  ?	flask_setavc_threshold		xsm/flask_op.h
>>>>  ?	flask_setenforce		xsm/flask_op.h
>>>> -!	flask_sid_context		xsm/flask_op.h
>>>>  ?	flask_transition		xsm/flask_op.h
>>>
>>> Shouldn't those become checks then?
>>
>> No, checking will never succeed for structures containing
>> XEN_GUEST_HANDLE(). But there's no point in generating xlat macros
>> when they're never used. There are two fundamentally different
>> strategies for handling the compat hypercalls: One is to wrap a
>> translation layer around the native hypercall. That's where the
>> xlat macros come into play. The other, used here, is to compile
>> the entire hypercall function a second time, arranging for the
>> compat structures to get used in place of the native ones. There
>> are no xlat macros involved here, all that's needed are correctly
>> translated structures. (For completeness, x86's MCA hypercall
>> uses yet another, quite adhoc strategy for handling, but also not
>> involving any xlat macro use. Hence the consideration there to
>> possibly drop the respective lines from the file here.)
> 
> Thanks, I think this explanation is helpful and I wonder whether it
> would be possible to have something along this lines in a file or as a
> comment somewhere, maybe at the top of xlat.lst?

To be honest - I'm not sure: Such a comment may indeed be helpful
to have, but I don't think I can see any single good place for it
to live. For people editing xlat.lst (a file the existence of which
many aren't even aware of), this would be a good place. But how
would others have any chance of running into this comment?

> Also could you add a line to the commit message noting that flask code
> doesn't use any of the translation macros because it follows a
> different approach to compat handling?

I've made the sentence start "Translation macros aren't used (and
hence needed) at all ..." - is that enough of an adjustment?

> For the patch:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

Jan
Roger Pau Monne July 15, 2020, 10:08 a.m. UTC | #5
On Wed, Jul 15, 2020 at 10:52:28AM +0200, Jan Beulich wrote:
> On 15.07.2020 10:41, Roger Pau Monné wrote:
> > On Wed, Jul 15, 2020 at 08:42:44AM +0200, Jan Beulich wrote:
> >> On 14.07.2020 16:58, Roger Pau Monné wrote:
> >>> On Wed, Jul 01, 2020 at 12:28:07PM +0200, Jan Beulich wrote:
> >>>> Translation macros aren't needed at all (or else a devicetree_label
> >>>> entry would have been missing), and userlist has been removed quite some
> >>>> time ago.
> >>>>
> >>>> No functional change.
> >>>>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>
> >>>> --- a/xen/include/xlat.lst
> >>>> +++ b/xen/include/xlat.lst
> >>>> @@ -148,14 +148,11 @@
> >>>>  ?	xenoprof_init			xenoprof.h
> >>>>  ?	xenoprof_passive		xenoprof.h
> >>>>  ?	flask_access			xsm/flask_op.h
> >>>> -!	flask_boolean			xsm/flask_op.h
> >>>>  ?	flask_cache_stats		xsm/flask_op.h
> >>>>  ?	flask_hash_stats		xsm/flask_op.h
> >>>> -!	flask_load			xsm/flask_op.h
> >>>>  ?	flask_ocontext			xsm/flask_op.h
> >>>>  ?	flask_peersid			xsm/flask_op.h
> >>>>  ?	flask_relabel			xsm/flask_op.h
> >>>>  ?	flask_setavc_threshold		xsm/flask_op.h
> >>>>  ?	flask_setenforce		xsm/flask_op.h
> >>>> -!	flask_sid_context		xsm/flask_op.h
> >>>>  ?	flask_transition		xsm/flask_op.h
> >>>
> >>> Shouldn't those become checks then?
> >>
> >> No, checking will never succeed for structures containing
> >> XEN_GUEST_HANDLE(). But there's no point in generating xlat macros
> >> when they're never used. There are two fundamentally different
> >> strategies for handling the compat hypercalls: One is to wrap a
> >> translation layer around the native hypercall. That's where the
> >> xlat macros come into play. The other, used here, is to compile
> >> the entire hypercall function a second time, arranging for the
> >> compat structures to get used in place of the native ones. There
> >> are no xlat macros involved here, all that's needed are correctly
> >> translated structures. (For completeness, x86's MCA hypercall
> >> uses yet another, quite adhoc strategy for handling, but also not
> >> involving any xlat macro use. Hence the consideration there to
> >> possibly drop the respective lines from the file here.)
> > 
> > Thanks, I think this explanation is helpful and I wonder whether it
> > would be possible to have something along this lines in a file or as a
> > comment somewhere, maybe at the top of xlat.lst?
> 
> To be honest - I'm not sure: Such a comment may indeed be helpful
> to have, but I don't think I can see any single good place for it
> to live. For people editing xlat.lst (a file the existence of which
> many aren't even aware of), this would be a good place. But how
> would others have any chance of running into this comment?

I would add it to xlat.lst rather than not adding it at all.

If we can find a better place to add it I'm all in, but as said I
would rather add it somewhere right now than just defer adding it
until the perfect placement is found.

> > Also could you add a line to the commit message noting that flask code
> > doesn't use any of the translation macros because it follows a
> > different approach to compat handling?
> 
> I've made the sentence start "Translation macros aren't used (and
> hence needed) at all ..." - is that enough of an adjustment?

Yes, that's fine.

Thanks.

Patch
diff mbox series

--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -148,14 +148,11 @@ 
 ?	xenoprof_init			xenoprof.h
 ?	xenoprof_passive		xenoprof.h
 ?	flask_access			xsm/flask_op.h
-!	flask_boolean			xsm/flask_op.h
 ?	flask_cache_stats		xsm/flask_op.h
 ?	flask_hash_stats		xsm/flask_op.h
-!	flask_load			xsm/flask_op.h
 ?	flask_ocontext			xsm/flask_op.h
 ?	flask_peersid			xsm/flask_op.h
 ?	flask_relabel			xsm/flask_op.h
 ?	flask_setavc_threshold		xsm/flask_op.h
 ?	flask_setenforce		xsm/flask_op.h
-!	flask_sid_context		xsm/flask_op.h
 ?	flask_transition		xsm/flask_op.h
--- a/xen/xsm/flask/flask_op.c
+++ b/xen/xsm/flask/flask_op.c
@@ -790,8 +790,6 @@  CHECK_flask_transition;
 #define xen_flask_load compat_flask_load
 #define flask_security_load compat_security_load
 
-#define xen_flask_userlist compat_flask_userlist
-
 #define xen_flask_sid_context compat_flask_sid_context
 #define flask_security_context compat_security_context
 #define flask_security_sid compat_security_sid