| Message ID | 5eda17da-7185-9cf6-7e87-70da57aa0ebc@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools: address Coverity UNUSED issues | expand |
On 12.06.23 13:46, Jan Beulich wrote: > The variable needs to be properly set only on the error paths. > > Coverity ID: 1532311 > Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID down to libxl") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Juergen Gross <jgross@suse.com> > --- > If XSM is disabled, is it really useful to issue the 2nd and 3rd calls > if the 1st yielded ENOSYS? Probably not. Juergen
On 6/12/23 07:46, Jan Beulich wrote: > The variable needs to be properly set only on the error paths. > > Coverity ID: 1532311 > Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID down to libxl") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.cm> > --- > If XSM is disabled, is it really useful to issue the 2nd and 3rd calls > if the 1st yielded ENOSYS? Would you be okay with the calls staying if instead on the first invocation of any libxl_flask_* method, flask status was checked and stored in a variable that would then be checked by any subsequent calls and immediately returned if flask was not enabled? v/r, dps
On 6/12/23 15:44, Daniel P. Smith wrote: > On 6/12/23 07:46, Jan Beulich wrote: >> The variable needs to be properly set only on the error paths. >> >> Coverity ID: 1532311 >> Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID >> down to libxl") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.cm> > >> --- >> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls >> if the 1st yielded ENOSYS? > > Would you be okay with the calls staying if instead on the first > invocation of any libxl_flask_* method, flask status was checked and > stored in a variable that would then be checked by any subsequent calls > and immediately returned if flask was not enabled? > > v/r, > dps Looking closer I realized there is a slight flaw in the logic here. The first call is accomplished via an xsm_op call and then assumes that FLASK is the only XSM that has implemented the xsm hook, xsm_op, and that the result will be an ENOSYS. If someone decides to implement an xsm_op hook for any of the existing XSM modules or introduces a new XSM module that has an xsm_op hook, the return likely would not be ENOSYS. I have often debated if there should be a way to query which XSM module was loaded for instances just like this. The question is what mechanism would be best to do so. v/r, dps
On 12.06.2023 22:21, Daniel P. Smith wrote: > > > On 6/12/23 15:44, Daniel P. Smith wrote: >> On 6/12/23 07:46, Jan Beulich wrote: >>> The variable needs to be properly set only on the error paths. >>> >>> Coverity ID: 1532311 >>> Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID >>> down to libxl") >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.cm> Thanks. >>> --- >>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls >>> if the 1st yielded ENOSYS? >> >> Would you be okay with the calls staying if instead on the first >> invocation of any libxl_flask_* method, flask status was checked and >> stored in a variable that would then be checked by any subsequent calls >> and immediately returned if flask was not enabled? I'm wary of global variables in shared libraries. > Looking closer I realized there is a slight flaw in the logic here. The > first call is accomplished via an xsm_op call and then assumes that > FLASK is the only XSM that has implemented the xsm hook, xsm_op, and > that the result will be an ENOSYS. If someone decides to implement an > xsm_op hook for any of the existing XSM modules or introduces a new XSM > module that has an xsm_op hook, the return likely would not be ENOSYS. I > have often debated if there should be a way to query which XSM module > was loaded for instances just like this. The question is what mechanism > would be best to do so. Well, this is what results from abusing ENOSYS. The default case of a switch() in a handler shouldn't return that value. Only if the entire hypercall is outright unimplemented, returning ENOSYS is appropriate. In fact I wonder whether dummy.h's xsm_do_xsm_op() is validly doing so, when that function also serves as the fallback for XSM modules choosing to not implement any of the op-s (like SILO does). Since in the specific case here it looks like the intention really is to carry out Flask-specific operations, a means to check for Flask specifically might indeed be appropriate. If not a new sub-op of xsm_op, a new sysctl might be another option. Jan
On 6/13/23 02:33, Jan Beulich wrote: > On 12.06.2023 22:21, Daniel P. Smith wrote: >> >> >> On 6/12/23 15:44, Daniel P. Smith wrote: >>> On 6/12/23 07:46, Jan Beulich wrote: >>>> The variable needs to be properly set only on the error paths. >>>> >>>> Coverity ID: 1532311 >>>> Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID >>>> down to libxl") >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.cm> > > Thanks. > My pleasure. >>>> --- >>>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls >>>> if the 1st yielded ENOSYS? >>> >>> Would you be okay with the calls staying if instead on the first >>> invocation of any libxl_flask_* method, flask status was checked and >>> stored in a variable that would then be checked by any subsequent calls >>> and immediately returned if flask was not enabled? > > I'm wary of global variables in shared libraries. > I agree with that sentiment, but I would distinguish global state from a global variable. I would take the approach of, static boot is_flask_enabled(void) { /* 0 unchecked, 1 checked but disabled, 2 enabled */ static int state = 0; if ( state == 0 ) state = check_flask_state(); /* pseudo call for real logic */ return (state < 2 ? false : true); } Then all the libxl_flask_* methods would is_flask_enabled(). This would not expose a global variable that could be manipulated by users of the library. >> Looking closer I realized there is a slight flaw in the logic here. The >> first call is accomplished via an xsm_op call and then assumes that >> FLASK is the only XSM that has implemented the xsm hook, xsm_op, and >> that the result will be an ENOSYS. If someone decides to implement an >> xsm_op hook for any of the existing XSM modules or introduces a new XSM >> module that has an xsm_op hook, the return likely would not be ENOSYS. I >> have often debated if there should be a way to query which XSM module >> was loaded for instances just like this. The question is what mechanism >> would be best to do so. > > Well, this is what results from abusing ENOSYS. The default case of a > switch() in a handler shouldn't return that value. Only if the entire > hypercall is outright unimplemented, returning ENOSYS is appropriate. > In fact I wonder whether dummy.h's xsm_do_xsm_op() is validly doing so, > when that function also serves as the fallback for XSM modules > choosing to not implement any of the op-s (like SILO does). I understand your point, but if ENOSYS (Not Implemented) is not correct, what is the appropriate return value for this module does not support this op? > Since in the specific case here it looks like the intention really is > to carry out Flask-specific operations, a means to check for Flask > specifically might indeed be appropriate. If not a new sub-op of > xsm_op, a new sysctl might be another option. I am actually split on whether this should be an sub-op of xsm op or if this should be exposed via hyperfs. I did not consider a sysctl, though I guess an argument could be constructed for it. v/r, dps
On 13.06.2023 11:21, Daniel P. Smith wrote: > On 6/13/23 02:33, Jan Beulich wrote: >> On 12.06.2023 22:21, Daniel P. Smith wrote: >>> On 6/12/23 15:44, Daniel P. Smith wrote: >>>> On 6/12/23 07:46, Jan Beulich wrote: >>>>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls >>>>> if the 1st yielded ENOSYS? >>>> >>>> Would you be okay with the calls staying if instead on the first >>>> invocation of any libxl_flask_* method, flask status was checked and >>>> stored in a variable that would then be checked by any subsequent calls >>>> and immediately returned if flask was not enabled? >> >> I'm wary of global variables in shared libraries. >> > > I agree with that sentiment, but I would distinguish global state from a > global variable. I would take the approach of, > > static boot is_flask_enabled(void) > { > /* 0 unchecked, 1 checked but disabled, 2 enabled */ > static int state = 0; > > if ( state == 0 ) > state = check_flask_state(); /* pseudo call for real logic */ > > return (state < 2 ? false : true); > } > > Then all the libxl_flask_* methods would is_flask_enabled(). This would > not expose a global variable that could be manipulated by users of the > library. Well, I certainly did assume the variable wouldn't be widely exposed. And the library also clearly is far from free of r/w data. But still. That aspect aside - wouldn't such a basic state determination better belong in libxc then? (There's far less contents in .data / .bss there.) >>> Looking closer I realized there is a slight flaw in the logic here. The >>> first call is accomplished via an xsm_op call and then assumes that >>> FLASK is the only XSM that has implemented the xsm hook, xsm_op, and >>> that the result will be an ENOSYS. If someone decides to implement an >>> xsm_op hook for any of the existing XSM modules or introduces a new XSM >>> module that has an xsm_op hook, the return likely would not be ENOSYS. I >>> have often debated if there should be a way to query which XSM module >>> was loaded for instances just like this. The question is what mechanism >>> would be best to do so. >> >> Well, this is what results from abusing ENOSYS. The default case of a >> switch() in a handler shouldn't return that value. Only if the entire >> hypercall is outright unimplemented, returning ENOSYS is appropriate. >> In fact I wonder whether dummy.h's xsm_do_xsm_op() is validly doing so, >> when that function also serves as the fallback for XSM modules >> choosing to not implement any of the op-s (like SILO does). > > I understand your point, but if ENOSYS (Not Implemented) is not correct, > what is the appropriate return value for this module does not support > this op? You almost say it by the wording you chose: EOPNOTSUPP. >> Since in the specific case here it looks like the intention really is >> to carry out Flask-specific operations, a means to check for Flask >> specifically might indeed be appropriate. If not a new sub-op of >> xsm_op, a new sysctl might be another option. > > I am actually split on whether this should be an sub-op of xsm op or if > this should be exposed via hyperfs. I did not consider a sysctl, though > I guess an argument could be constructed for it. Please don't forget that hypfs, unlike sysctl, is an optional thing, so you can't use it for decision taking (but only for informational purposes). Jan
On 6/13/23 05:40, Jan Beulich wrote: > On 13.06.2023 11:21, Daniel P. Smith wrote: >> On 6/13/23 02:33, Jan Beulich wrote: >>> On 12.06.2023 22:21, Daniel P. Smith wrote: >>>> On 6/12/23 15:44, Daniel P. Smith wrote: >>>>> On 6/12/23 07:46, Jan Beulich wrote: >>>>>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls >>>>>> if the 1st yielded ENOSYS? >>>>> >>>>> Would you be okay with the calls staying if instead on the first >>>>> invocation of any libxl_flask_* method, flask status was checked and >>>>> stored in a variable that would then be checked by any subsequent calls >>>>> and immediately returned if flask was not enabled? >>> >>> I'm wary of global variables in shared libraries. >>> >> >> I agree with that sentiment, but I would distinguish global state from a >> global variable. I would take the approach of, >> >> static boot is_flask_enabled(void) >> { >> /* 0 unchecked, 1 checked but disabled, 2 enabled */ >> static int state = 0; >> >> if ( state == 0 ) >> state = check_flask_state(); /* pseudo call for real logic */ >> >> return (state < 2 ? false : true); >> } >> >> Then all the libxl_flask_* methods would is_flask_enabled(). This would >> not expose a global variable that could be manipulated by users of the >> library. > > Well, I certainly did assume the variable wouldn't be widely exposed. > And the library also clearly is far from free of r/w data. But still. > > That aspect aside - wouldn't such a basic state determination better > belong in libxc then? (There's far less contents in .data / .bss > there.) Not opposed at all, so a series with a patch to libxc paired and a new sub-op/sysctl as discussed below would be acceptable? >>>> Looking closer I realized there is a slight flaw in the logic here. The >>>> first call is accomplished via an xsm_op call and then assumes that >>>> FLASK is the only XSM that has implemented the xsm hook, xsm_op, and >>>> that the result will be an ENOSYS. If someone decides to implement an >>>> xsm_op hook for any of the existing XSM modules or introduces a new XSM >>>> module that has an xsm_op hook, the return likely would not be ENOSYS. I >>>> have often debated if there should be a way to query which XSM module >>>> was loaded for instances just like this. The question is what mechanism >>>> would be best to do so. >>> >>> Well, this is what results from abusing ENOSYS. The default case of a >>> switch() in a handler shouldn't return that value. Only if the entire >>> hypercall is outright unimplemented, returning ENOSYS is appropriate. >>> In fact I wonder whether dummy.h's xsm_do_xsm_op() is validly doing so, >>> when that function also serves as the fallback for XSM modules >>> choosing to not implement any of the op-s (like SILO does). >> >> I understand your point, but if ENOSYS (Not Implemented) is not correct, >> what is the appropriate return value for this module does not support >> this op? > > You almost say it by the wording you chose: EOPNOTSUPP. > Erg, you are right, not sure why it didn't click. Though I guess what should be used will be moot if the decision is to add an xsm-op subop to dummy to support reporting the current XSM module. >>> Since in the specific case here it looks like the intention really is >>> to carry out Flask-specific operations, a means to check for Flask >>> specifically might indeed be appropriate. If not a new sub-op of >>> xsm_op, a new sysctl might be another option. >> >> I am actually split on whether this should be an sub-op of xsm op or if >> this should be exposed via hyperfs. I did not consider a sysctl, though >> I guess an argument could be constructed for it. > > Please don't forget that hypfs, unlike sysctl, is an optional thing, > so you can't use it for decision taking (but only for informational > purposes). Good point regarding hypfs, the check mentioned above is expected to always work, thus an optional feature probably is not best. The next question is, should it be sysctl or xsm-op. I am leaning towards xsm-op because as much as I believe XSM should be consider core to Xen, the XSM logic should remain contained. Adding it to sysctl would mean having to expose XSM state outside of XSM code and would make sysctl logic have to be aware of XSM state query functions. With that said, if someone wants to make the case for sysctl, I am open to considering it. v/r, dps
On 13.06.2023 11:57, Daniel P. Smith wrote: > On 6/13/23 05:40, Jan Beulich wrote: >> On 13.06.2023 11:21, Daniel P. Smith wrote: >>> On 6/13/23 02:33, Jan Beulich wrote: >>>> On 12.06.2023 22:21, Daniel P. Smith wrote: >>>>> On 6/12/23 15:44, Daniel P. Smith wrote: >>>>>> On 6/12/23 07:46, Jan Beulich wrote: >>>>>>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls >>>>>>> if the 1st yielded ENOSYS? >>>>>> >>>>>> Would you be okay with the calls staying if instead on the first >>>>>> invocation of any libxl_flask_* method, flask status was checked and >>>>>> stored in a variable that would then be checked by any subsequent calls >>>>>> and immediately returned if flask was not enabled? >>>> >>>> I'm wary of global variables in shared libraries. >>>> >>> >>> I agree with that sentiment, but I would distinguish global state from a >>> global variable. I would take the approach of, >>> >>> static boot is_flask_enabled(void) >>> { >>> /* 0 unchecked, 1 checked but disabled, 2 enabled */ >>> static int state = 0; >>> >>> if ( state == 0 ) >>> state = check_flask_state(); /* pseudo call for real logic */ >>> >>> return (state < 2 ? false : true); >>> } >>> >>> Then all the libxl_flask_* methods would is_flask_enabled(). This would >>> not expose a global variable that could be manipulated by users of the >>> library. >> >> Well, I certainly did assume the variable wouldn't be widely exposed. >> And the library also clearly is far from free of r/w data. But still. >> >> That aspect aside - wouldn't such a basic state determination better >> belong in libxc then? (There's far less contents in .data / .bss >> there.) > > Not opposed at all, so a series with a patch to libxc paired and a new > sub-op/sysctl as discussed below would be acceptable? I can only say yes here for the hypervisor side; I'm not a maintainer of any significant part of the tool stack. >>>> Since in the specific case here it looks like the intention really is >>>> to carry out Flask-specific operations, a means to check for Flask >>>> specifically might indeed be appropriate. If not a new sub-op of >>>> xsm_op, a new sysctl might be another option. >>> >>> I am actually split on whether this should be an sub-op of xsm op or if >>> this should be exposed via hyperfs. I did not consider a sysctl, though >>> I guess an argument could be constructed for it. >> >> Please don't forget that hypfs, unlike sysctl, is an optional thing, >> so you can't use it for decision taking (but only for informational >> purposes). > > Good point regarding hypfs, the check mentioned above is expected to > always work, thus an optional feature probably is not best. > > The next question is, should it be sysctl or xsm-op. I am leaning > towards xsm-op because as much as I believe XSM should be consider core > to Xen, the XSM logic should remain contained. Adding it to sysctl would > mean having to expose XSM state outside of XSM code and would make > sysctl logic have to be aware of XSM state query functions. Well, you seemed hesitant towards an xsm-sub-op, so I suggested sysctl as a possible alternative. One possible issue with an xsm-op is that at the public interface level (public headers), besides __HYPERVISOR_xsm_op there's no notion of xsm-op. And it was pointed out before that by kind of blindly wiring xsm_op through to flask_op, adding XSM-module- independent ops and/or ops for some future non-Flask module is going to be a little, well, bumpy. Jan
On 6/13/23 06:15, Jan Beulich wrote: > On 13.06.2023 11:57, Daniel P. Smith wrote: >> On 6/13/23 05:40, Jan Beulich wrote: >>> On 13.06.2023 11:21, Daniel P. Smith wrote: >>>> On 6/13/23 02:33, Jan Beulich wrote: >>>>> On 12.06.2023 22:21, Daniel P. Smith wrote: >>>>>> On 6/12/23 15:44, Daniel P. Smith wrote: >>>>>>> On 6/12/23 07:46, Jan Beulich wrote: >>>>>>>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls >>>>>>>> if the 1st yielded ENOSYS? >>>>>>> >>>>>>> Would you be okay with the calls staying if instead on the first >>>>>>> invocation of any libxl_flask_* method, flask status was checked and >>>>>>> stored in a variable that would then be checked by any subsequent calls >>>>>>> and immediately returned if flask was not enabled? >>>>> >>>>> I'm wary of global variables in shared libraries. >>>>> >>>> >>>> I agree with that sentiment, but I would distinguish global state from a >>>> global variable. I would take the approach of, >>>> >>>> static boot is_flask_enabled(void) >>>> { >>>> /* 0 unchecked, 1 checked but disabled, 2 enabled */ >>>> static int state = 0; >>>> >>>> if ( state == 0 ) >>>> state = check_flask_state(); /* pseudo call for real logic */ >>>> >>>> return (state < 2 ? false : true); >>>> } >>>> >>>> Then all the libxl_flask_* methods would is_flask_enabled(). This would >>>> not expose a global variable that could be manipulated by users of the >>>> library. >>> >>> Well, I certainly did assume the variable wouldn't be widely exposed. >>> And the library also clearly is far from free of r/w data. But still. >>> >>> That aspect aside - wouldn't such a basic state determination better >>> belong in libxc then? (There's far less contents in .data / .bss >>> there.) >> >> Not opposed at all, so a series with a patch to libxc paired and a new >> sub-op/sysctl as discussed below would be acceptable? > > I can only say yes here for the hypervisor side; I'm not a maintainer > of any significant part of the tool stack. > Understood, thank you. >>>>> Since in the specific case here it looks like the intention really is >>>>> to carry out Flask-specific operations, a means to check for Flask >>>>> specifically might indeed be appropriate. If not a new sub-op of >>>>> xsm_op, a new sysctl might be another option. >>>> >>>> I am actually split on whether this should be an sub-op of xsm op or if >>>> this should be exposed via hyperfs. I did not consider a sysctl, though >>>> I guess an argument could be constructed for it. >>> >>> Please don't forget that hypfs, unlike sysctl, is an optional thing, >>> so you can't use it for decision taking (but only for informational >>> purposes). >> >> Good point regarding hypfs, the check mentioned above is expected to >> always work, thus an optional feature probably is not best. >> >> The next question is, should it be sysctl or xsm-op. I am leaning >> towards xsm-op because as much as I believe XSM should be consider core >> to Xen, the XSM logic should remain contained. Adding it to sysctl would >> mean having to expose XSM state outside of XSM code and would make >> sysctl logic have to be aware of XSM state query functions. > > Well, you seemed hesitant towards an xsm-sub-op, so I suggested sysctl > as a possible alternative. One possible issue with an xsm-op is that at > the public interface level (public headers), besides __HYPERVISOR_xsm_op > there's no notion of xsm-op. And it was pointed out before that by kind > of blindly wiring xsm_op through to flask_op, adding XSM-module- > independent ops and/or ops for some future non-Flask module is going to > be a little, well, bumpy. That is in fact the source of my hesitance. Fixing xsm sub-op from being a flask specific, not well documented thing, to a documented general interface is on the list of many improvements needed. I believe handling this case could be done as an incremental step towards that by introducing a common sub-op structure that is used for the "get current module" sub-op since it will be new sub-op for FLASK module as well. As you mentioned, the next step will be the bumpy one of trying to convert the existing FLASK xsm-op sub-ops over to the new common structure. Until I try, I won't know for sure if I can do this as a small step. Let me see if I can carve out some time and give it a try. v/r, dps
On Mon, Jun 12, 2023 at 01:46:19PM +0200, Jan Beulich wrote: > The variable needs to be properly set only on the error paths. > > Coverity ID: 1532311 > Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID down to libxl") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Anthony PERARD <anthony.perard@citrix.com> Thanks,
--- a/tools/libs/light/libxl_create.c +++ b/tools/libs/light/libxl_create.c @@ -1081,13 +1079,12 @@ int libxl__domain_config_setdefault(libx ret = libxl_flask_context_to_sid(ctx, s, strlen(s), &d_config->c_info.ssidref); if (ret) { - if (errno == ENOSYS) { - LOGD(WARN, domid, "XSM Disabled: init_seclabel not supported"); - ret = 0; - } else { + if (errno != ENOSYS) { LOGD(ERROR, domid, "Invalid init_seclabel: %s", s); goto error_out; } + + LOGD(WARN, domid, "XSM Disabled: init_seclabel not supported"); } } @@ -1096,13 +1093,12 @@ int libxl__domain_config_setdefault(libx ret = libxl_flask_context_to_sid(ctx, s, strlen(s), &d_config->b_info.exec_ssidref); if (ret) { - if (errno == ENOSYS) { - LOGD(WARN, domid, "XSM Disabled: seclabel not supported"); - ret = 0; - } else { + if (errno != ENOSYS) { LOGD(ERROR, domid, "Invalid seclabel: %s", s); goto error_out; } + + LOGD(WARN, domid, "XSM Disabled: seclabel not supported"); } } @@ -1111,14 +1107,13 @@ int libxl__domain_config_setdefault(libx ret = libxl_flask_context_to_sid(ctx, s, strlen(s), &d_config->b_info.device_model_ssidref); if (ret) { - if (errno == ENOSYS) { - LOGD(WARN, domid, - "XSM Disabled: device_model_stubdomain_seclabel not supported"); - ret = 0; - } else { + if (errno != ENOSYS) { LOGD(ERROR, domid, "Invalid device_model_stubdomain_seclabel: %s", s); goto error_out; } + + LOGD(WARN, domid, + "XSM Disabled: device_model_stubdomain_seclabel not supported"); } }
The variable needs to be properly set only on the error paths. Coverity ID: 1532311 Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID down to libxl") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- If XSM is disabled, is it really useful to issue the 2nd and 3rd calls if the 1st yielded ENOSYS?