diff mbox series

[v5,4/4] umip_basic_test.c: improve kconfig verification to avoid umip wrong abort case

Message ID 20191220092529.3239-4-pengfei.xu@intel.com
State New
Headers show
Series [v5,1/4] lib/tst_kconfig.c: add any kconfig with or without expected value function | expand

Commit Message

Xu, Pengfei Dec. 20, 2019, 9:25 a.m. UTC
From v5.5 main line, umip kconfig changed from "CONFIG_X86_INTEL_UMIP=y"
to "CONFIG_X86_UMIP=y".
We could use "CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y" to check kconfig
CONFIG_X86_INTEL_UMIP=y(old kernel) or CONFIG_X86_UMIP=y(new kernel) for umip.

Signed-off-by: Pengfei Xu <pengfei.xu@intel.com>
---
 testcases/kernel/security/umip/umip_basic_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Petr Vorel May 25, 2020, 9:24 p.m. UTC | #1
Hi Xu,

> From v5.5 main line, umip kconfig changed from "CONFIG_X86_INTEL_UMIP=y"
> to "CONFIG_X86_UMIP=y".
> We could use "CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y" to check kconfig
> CONFIG_X86_INTEL_UMIP=y(old kernel) or CONFIG_X86_UMIP=y(new kernel) for umip.

> Signed-off-by: Pengfei Xu <pengfei.xu@intel.com>
> ---
>  testcases/kernel/security/umip/umip_basic_test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

> diff --git a/testcases/kernel/security/umip/umip_basic_test.c b/testcases/kernel/security/umip/umip_basic_test.c
> index 1baa26c52..24eca8890 100644
> --- a/testcases/kernel/security/umip/umip_basic_test.c
> +++ b/testcases/kernel/security/umip/umip_basic_test.c
> @@ -171,7 +171,7 @@ static struct tst_test test = {
>  	.forks_child = 1,
>  	.test = verify_umip_instruction,
>  	.needs_kconfigs = (const char *[]){
> -		"CONFIG_X86_INTEL_UMIP=y",
> +		"CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y",

We're sorry to get to your patch now, after 5 months.

Thanks for a report and your effort to fix the problem. But this does not work,
because current implementation does not support '|' as bitwise or, with this
patch will result on tests being skipped for both cases.

While it'd be easy to implement support for bitwise or in tst_kconfig_read(),
it might be enough just to check for kernel version:

.needs_kconfigs = (const char *[]){
#if LINUX_VERSION_CODE < KERNEL_VERSION(5, 5, 0)
	"CONFIG_X86_INTEL_UMIP=y",
#else
	"CONFIG_X86_UMIP=y",
#endif

But that will work unless this feature is not backported (IMHO commit
b971880fe79f ("x86/Kconfig: Rename UMIP config parameter") is kind of cleanup,
therefore unlikely to be backported, but it can happen).

Kind regards,
Petr
Xu, Pengfei May 26, 2020, 2:32 a.m. UTC | #2
Hi Petr,
  Thanks and my feedback is as below.

  Thanks.
  BR.

On 2020-05-25 at 23:24:01 +0200, Petr Vorel wrote:
> Hi Xu,
> 
> > From v5.5 main line, umip kconfig changed from "CONFIG_X86_INTEL_UMIP=y"
> > to "CONFIG_X86_UMIP=y".
> > We could use "CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y" to check kconfig
> > CONFIG_X86_INTEL_UMIP=y(old kernel) or CONFIG_X86_UMIP=y(new kernel) for umip.
> 
> > Signed-off-by: Pengfei Xu <pengfei.xu@intel.com>
> > ---
> >  testcases/kernel/security/umip/umip_basic_test.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> > diff --git a/testcases/kernel/security/umip/umip_basic_test.c b/testcases/kernel/security/umip/umip_basic_test.c
> > index 1baa26c52..24eca8890 100644
> > --- a/testcases/kernel/security/umip/umip_basic_test.c
> > +++ b/testcases/kernel/security/umip/umip_basic_test.c
> > @@ -171,7 +171,7 @@ static struct tst_test test = {
> >  	.forks_child = 1,
> >  	.test = verify_umip_instruction,
> >  	.needs_kconfigs = (const char *[]){
> > -		"CONFIG_X86_INTEL_UMIP=y",
> > +		"CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y",
> 
> We're sorry to get to your patch now, after 5 months.
> 
> Thanks for a report and your effort to fix the problem. But this does not work,
> because current implementation does not support '|' as bitwise or, with this
> patch will result on tests being skipped for both cases.
  CONFIG_A|CONFIG_B=y means CONGIG_A or CONGIG_B equal 'y', it will meet the
  test condition. So it's as expected; only could not find CONFIG_A and
  CONFIG_B equal to 'y', then it will not meet the test condition and exit.
  It should be as expected.
  Thank you for considering this patch again.

> 
> While it'd be easy to implement support for bitwise or in tst_kconfig_read(),
> it might be enough just to check for kernel version:
> 
> .needs_kconfigs = (const char *[]){
> #if LINUX_VERSION_CODE < KERNEL_VERSION(5, 5, 0)
> 	"CONFIG_X86_INTEL_UMIP=y",
> #else
> 	"CONFIG_X86_UMIP=y",
> #endif
> 
> But that will work unless this feature is not backported (IMHO commit
> b971880fe79f ("x86/Kconfig: Rename UMIP config parameter") is kind of cleanup,
> therefore unlikely to be backported, but it can happen).
> 
> Kind regards,
> Petr
Petr Vorel May 26, 2020, 9:23 a.m. UTC | #3
Hi Xu,

...
> > Thanks for a report and your effort to fix the problem. But this does not work,
> > because current implementation does not support '|' as bitwise or, with this
> > patch will result on tests being skipped for both cases.
>   CONFIG_A|CONFIG_B=y means CONGIG_A or CONGIG_B equal 'y', it will meet the
>   test condition. So it's as expected; only could not find CONFIG_A and
>   CONFIG_B equal to 'y', then it will not meet the test condition and exit.
>   It should be as expected.
>   Thank you for considering this patch again.

Well, I understand your syntax, that you mean | as bitwise or :).
But where did you find that this syntax is supported? Have a look in
tst_kconfig_read() implementation (lib/tst_kconfig.c), there is nothing like
this. And, indeed, if you test your patch on both CONFIG_X86_INTEL_UMIP=y and
CONFIG_X86_UMIP=y, it end up with:

tst_kconfig.c:252: INFO: Missing kernel CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y
tst_kconfig.c:284: CONF: Aborting due to unsuitable kernel config, see above!

which confirm my statement there is no bitwise or support implemented :).
Or am I missing something?

And it might be questionable if CONFIG_A|CONFIG_B=y would mean (CONFIG_A|CONFIG_B)=y
or (CONFIG_A|CONFIG_B=y), thus it should be CONFIG_A=y|CONFIG_B=y

But given the fact this functionality is needed just for a single test and can
be workarounded I suggested to use LINUX_VERSION_CODE instead.
The only problem can happen when this is backported to the old code. But we
could also try to detect that with custom call twice tst_kconfig_read() in the
test setup.

Kind regards,
Petr

> > While it'd be easy to implement support for bitwise or in tst_kconfig_read(),
> > it might be enough just to check for kernel version:

> > .needs_kconfigs = (const char *[]) {
> > #if LINUX_VERSION_CODE < KERNEL_VERSION(5, 5, 0)
> > 	"CONFIG_X86_INTEL_UMIP=y",
> > #else
> > 	"CONFIG_X86_UMIP=y",
> > #endif

> > But that will work unless this feature is not backported (IMHO commit
> > b971880fe79f ("x86/Kconfig: Rename UMIP config parameter") is kind of cleanup,
> > therefore unlikely to be backported, but it can happen).

> > Kind regards,
> > Petr
Petr Vorel May 26, 2020, 9:27 a.m. UTC | #4
Hi Xu,

> ...
> > > Thanks for a report and your effort to fix the problem. But this does not work,
> > > because current implementation does not support '|' as bitwise or, with this
> > > patch will result on tests being skipped for both cases.
> >   CONFIG_A|CONFIG_B=y means CONGIG_A or CONGIG_B equal 'y', it will meet the
> >   test condition. So it's as expected; only could not find CONFIG_A and
> >   CONFIG_B equal to 'y', then it will not meet the test condition and exit.
> >   It should be as expected.
> >   Thank you for considering this patch again.

> Well, I understand your syntax, that you mean | as bitwise or :).
> But where did you find that this syntax is supported? Have a look in
> tst_kconfig_read() implementation (lib/tst_kconfig.c), there is nothing like
> this. And, indeed, if you test your patch on both CONFIG_X86_INTEL_UMIP=y and
> CONFIG_X86_UMIP=y, it end up with:

> tst_kconfig.c:252: INFO: Missing kernel CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y
> tst_kconfig.c:284: CONF: Aborting due to unsuitable kernel config, see above!

> which confirm my statement there is no bitwise or support implemented :).
> Or am I missing something?

OK I now realized, that it's a 4th patch of patchset. I thought it's just single
patch, because the rest was rejected by Cyril:
https://patchwork.ozlabs.org/project/ltp/list/?series=149804&state=*

But it looks like Cyril is not against the implementation, it just needs to be
fixed:
https://patchwork.ozlabs.org/comment/2352151/

Kind regards,
Petr
Xu, Pengfei May 26, 2020, 10:07 a.m. UTC | #5
Hi Petr,

On 2020-05-26 at 11:27:03 +0200, Petr Vorel wrote:
> Hi Xu,
> 
> > ...
> > > > Thanks for a report and your effort to fix the problem. But this does not work,
> > > > because current implementation does not support '|' as bitwise or, with this
> > > > patch will result on tests being skipped for both cases.
> > >   CONFIG_A|CONFIG_B=y means CONGIG_A or CONGIG_B equal 'y', it will meet the
> > >   test condition. So it's as expected; only could not find CONFIG_A and
> > >   CONFIG_B equal to 'y', then it will not meet the test condition and exit.
> > >   It should be as expected.
> > >   Thank you for considering this patch again.
> 
> > Well, I understand your syntax, that you mean | as bitwise or :).
> > But where did you find that this syntax is supported? Have a look in
> > tst_kconfig_read() implementation (lib/tst_kconfig.c), there is nothing like
> > this. And, indeed, if you test your patch on both CONFIG_X86_INTEL_UMIP=y and
> > CONFIG_X86_UMIP=y, it end up with:
> 
> > tst_kconfig.c:252: INFO: Missing kernel CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y
> > tst_kconfig.c:284: CONF: Aborting due to unsuitable kernel config, see above!
> 
> > which confirm my statement there is no bitwise or support implemented :).
> > Or am I missing something?
> 
> OK I now realized, that it's a 4th patch of patchset. I thought it's just single
> patch, because the rest was rejected by Cyril:
> https://patchwork.ozlabs.org/project/ltp/list/?series=149804&state=*
> 
> But it looks like Cyril is not against the implementation, it just needs to be
> fixed:
> https://patchwork.ozlabs.org/comment/2352151/

You are right, actually it could be worked as my suggest way:
"CONFIG_A|CONFIG_B=Y".
I tried to use Cyril's advice "CONFIG_A=X|CONFIG_B=Y" way, which will
add more code complexity, so I just want to solve the problem I am currently
facing.
If we really need the "CONFIG_A=X|CONFIG_B=Y" function, which cannot be
satisfied by "CONFIG_A|CONFIG_B=Y" function in the future, then we could add
this function I think.
Thanks for your considering.

BR
Thanks!

Pengfei

> 
> Kind regards,
> Petr
Petr Vorel May 26, 2020, 10:11 a.m. UTC | #6
Hi Pengfei,

> > But it looks like Cyril is not against the implementation, it just needs to be
> > fixed:
> > https://patchwork.ozlabs.org/comment/2352151/

> You are right, actually it could be worked as my suggest way:
> "CONFIG_A|CONFIG_B=Y".
> I tried to use Cyril's advice "CONFIG_A=X|CONFIG_B=Y" way, which will
> add more code complexity, so I just want to solve the problem I am currently
> facing.
> If we really need the "CONFIG_A=X|CONFIG_B=Y" function, which cannot be
> satisfied by "CONFIG_A|CONFIG_B=Y" function in the future, then we could add
> this function I think.
> Thanks for your considering.

I'd also think that we need "CONFIG_A=X|CONFIG_B=Y", because
"CONFIG_A|CONFIG_B=Y" is ambiguous (we support both CONFIG_FOO and
CONFIG_FOO=bar and this must stay even with |).

Will you send a patch for that or shell I fix it with LINUX_VERSION_CODE <
KERNEL_VERSION for now?

Kind regards,
Petr
Xu, Pengfei May 26, 2020, 10:37 a.m. UTC | #7
Hi Petr,

On 2020-05-26 at 12:11:33 +0200, Petr Vorel wrote:
> Hi Pengfei,
> 
> > > But it looks like Cyril is not against the implementation, it just needs to be
> > > fixed:
> > > https://patchwork.ozlabs.org/comment/2352151/
> 
> > You are right, actually it could be worked as my suggest way:
> > "CONFIG_A|CONFIG_B=Y".
> > I tried to use Cyril's advice "CONFIG_A=X|CONFIG_B=Y" way, which will
> > add more code complexity, so I just want to solve the problem I am currently
> > facing.
> > If we really need the "CONFIG_A=X|CONFIG_B=Y" function, which cannot be
> > satisfied by "CONFIG_A|CONFIG_B=Y" function in the future, then we could add
> > this function I think.
> > Thanks for your considering.
> 
> I'd also think that we need "CONFIG_A=X|CONFIG_B=Y", because
> "CONFIG_A|CONFIG_B=Y" is ambiguous (we support both CONFIG_FOO and
> CONFIG_FOO=bar and this must stay even with |).
> 
> Will you send a patch for that or shell I fix it with LINUX_VERSION_CODE <
> KERNEL_VERSION for now?

  Ok, thanks for your LINUX_VERSION way to fix this issue.
  Thanks!
  BR.

> 
> Kind regards,
> Petr
Xu, Pengfei May 27, 2020, 1:22 a.m. UTC | #8
Hi Petr,
  Seems LINUX_VERSION_CODE way it not suitable when test platform is not compiled platform.
  Need to use " if ((tst_kvercmp(5, 5, 0)) >= 0)" way.

Thanks!
BR
Pengfei
+86 021 61164364   



-----Original Message-----
From: Pengfei Xu <pengfei.xu@intel.com> 
Sent: Tuesday, May 26, 2020 18:37
To: Petr Vorel <pvorel@suse.cz>
Cc: ltp <ltp@lists.linux.it>; Neri, Ricardo <ricardo.neri@intel.com>; Su, Heng <heng.su@intel.com>; Kasten, Robert A <robert.a.kasten@intel.com>; Cyril Hrubis <chrubis@suse.cz>; Jan Stancek <jstancek@redhat.com>; Li Wang <liwang@redhat.com>
Subject: Re: [LTP] [PATCH v5 4/4] umip_basic_test.c: improve kconfig verification to avoid umip wrong abort case

Hi Petr,

On 2020-05-26 at 12:11:33 +0200, Petr Vorel wrote:
> Hi Pengfei,
> 
> > > But it looks like Cyril is not against the implementation, it just 
> > > needs to be
> > > fixed:
> > > https://patchwork.ozlabs.org/comment/2352151/
> 
> > You are right, actually it could be worked as my suggest way:
> > "CONFIG_A|CONFIG_B=Y".
> > I tried to use Cyril's advice "CONFIG_A=X|CONFIG_B=Y" way, which 
> > will add more code complexity, so I just want to solve the problem I 
> > am currently facing.
> > If we really need the "CONFIG_A=X|CONFIG_B=Y" function, which cannot 
> > be satisfied by "CONFIG_A|CONFIG_B=Y" function in the future, then 
> > we could add this function I think.
> > Thanks for your considering.
> 
> I'd also think that we need "CONFIG_A=X|CONFIG_B=Y", because 
> "CONFIG_A|CONFIG_B=Y" is ambiguous (we support both CONFIG_FOO and 
> CONFIG_FOO=bar and this must stay even with |).
> 
> Will you send a patch for that or shell I fix it with 
> LINUX_VERSION_CODE < KERNEL_VERSION for now?

  Ok, thanks for your LINUX_VERSION way to fix this issue.
  Thanks!
  BR.

> 
> Kind regards,
> Petr
Petr Vorel May 27, 2020, 6:26 a.m. UTC | #9
Hi Pengfei,

>   Seems LINUX_VERSION_CODE way it not suitable when test platform is not compiled platform.
Well, you're expected to have installed kernel headers for target platform, when
you cross compile. That's what embedded distros like buildroot or yocto do.
We already use constructs like this in the code.

>   Need to use " if ((tst_kvercmp(5, 5, 0)) >= 0)" way.
As you noticed in previous mail, this will not work, as it's code outside of the
function, so you cannot call any function.

Kind regards,
Petr
Xu, Pengfei May 27, 2020, 12:24 p.m. UTC | #10
Hi Petr,
   Ok, thanks!

Thanks!
BR
Pengfei
+86 021 61164364   



-----Original Message-----
From: Petr Vorel <pvorel@suse.cz> 
Sent: Wednesday, May 27, 2020 14:27
To: Xu, Pengfei <pengfei.xu@intel.com>
Cc: ltp <ltp@lists.linux.it>; Neri, Ricardo <ricardo.neri@intel.com>; Su, Heng <heng.su@intel.com>; Kasten, Robert A <robert.a.kasten@intel.com>; Cyril Hrubis <chrubis@suse.cz>; Jan Stancek <jstancek@redhat.com>; Li Wang <liwang@redhat.com>
Subject: Re: [LTP] [PATCH v5 4/4] umip_basic_test.c: improve kconfig verification to avoid umip wrong abort case

Hi Pengfei,

>   Seems LINUX_VERSION_CODE way it not suitable when test platform is not compiled platform.
Well, you're expected to have installed kernel headers for target platform, when you cross compile. That's what embedded distros like buildroot or yocto do.
We already use constructs like this in the code.

>   Need to use " if ((tst_kvercmp(5, 5, 0)) >= 0)" way.
As you noticed in previous mail, this will not work, as it's code outside of the function, so you cannot call any function.

Kind regards,
Petr
diff mbox series

Patch

diff --git a/testcases/kernel/security/umip/umip_basic_test.c b/testcases/kernel/security/umip/umip_basic_test.c
index 1baa26c52..24eca8890 100644
--- a/testcases/kernel/security/umip/umip_basic_test.c
+++ b/testcases/kernel/security/umip/umip_basic_test.c
@@ -171,7 +171,7 @@  static struct tst_test test = {
 	.forks_child = 1,
 	.test = verify_umip_instruction,
 	.needs_kconfigs = (const char *[]){
-		"CONFIG_X86_INTEL_UMIP=y",
+		"CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y",
 		NULL
 	},
 	.needs_root = 1,