diff mbox series

[5/8] kselftest/arm64: mte: fix printf type warning about mask

Message ID 20240816153251.2833702-6-andre.przywara@arm.com (mailing list archive)
State New
Headers show
Series kselftest/arm64: various compilation fixes | expand

Commit Message

Andre Przywara Aug. 16, 2024, 3:32 p.m. UTC
When masking the return value of a prctl, which is clearly an "int", we
use a uapi header provided mask, which is defined using an "UL" modifier,
so the whole expression is promoted to a long. This upsets the compiler's
printf type checker, because we use "%x" in the format string.

While we could simply upgrade this to a "%lx", it sounds wrong to
promote the "ret" variable, that is clearly an int.
Downcast the mask instead, to keep the type correct.

Fixes: e2d9642a5a51 ("kselftest/arm64: Add simple test for MTE prctl")
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/testing/selftests/arm64/mte/check_prctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Mark Brown Aug. 16, 2024, 4:30 p.m. UTC | #1
On Fri, Aug 16, 2024 at 04:32:48PM +0100, Andre Przywara wrote:
> When masking the return value of a prctl, which is clearly an "int", we
> use a uapi header provided mask, which is defined using an "UL" modifier,
> so the whole expression is promoted to a long. This upsets the compiler's
> printf type checker, because we use "%x" in the format string.
> 
> While we could simply upgrade this to a "%lx", it sounds wrong to
> promote the "ret" variable, that is clearly an int.
> Downcast the mask instead, to keep the type correct.

This suggests that we've got some confusion with the UAPI, these flags
need to go through a prctl() return so they shouldn't be unsigned
long...  That said, it's UAPI so I'm not sure that's fixable.

> -	if ((ret & PR_MTE_TCF_MASK) == mask) {
> +	if ((ret & (int)PR_MTE_TCF_MASK) == mask) {
>  		ksft_test_result_pass("%s\n", name);
>  	} else {
>  		ksft_print_msg("Got %x, expected %x\n",
> -			       (ret & PR_MTE_TCF_MASK), mask);
> +			       (ret & (int)PR_MTE_TCF_MASK), mask);
>  		ksft_test_result_fail("%s\n", name);

TBH my inclination is that this is worse than letting the value be
promoted, casts (particularly casts of constants) are just obviously
suspect in a way that printf() formats aren't.
Andre Przywara Aug. 16, 2024, 4:55 p.m. UTC | #2
On Fri, 16 Aug 2024 17:30:14 +0100
Mark Brown <broonie@kernel.org> wrote:

Hi Mark,

thanks for having a look!

> On Fri, Aug 16, 2024 at 04:32:48PM +0100, Andre Przywara wrote:
> > When masking the return value of a prctl, which is clearly an "int", we
> > use a uapi header provided mask, which is defined using an "UL" modifier,
> > so the whole expression is promoted to a long. This upsets the compiler's
> > printf type checker, because we use "%x" in the format string.
> > 
> > While we could simply upgrade this to a "%lx", it sounds wrong to
> > promote the "ret" variable, that is clearly an int.
> > Downcast the mask instead, to keep the type correct.  
> 
> This suggests that we've got some confusion with the UAPI, these flags
> need to go through a prctl() return so they shouldn't be unsigned
> long...  That said, it's UAPI so I'm not sure that's fixable.

My thoughts, exactly. Not sure if this mask is used on some larger types
somewhere else, but it's indeed moot since it's UAPI.

> > -	if ((ret & PR_MTE_TCF_MASK) == mask) {
> > +	if ((ret & (int)PR_MTE_TCF_MASK) == mask) {
> >  		ksft_test_result_pass("%s\n", name);
> >  	} else {
> >  		ksft_print_msg("Got %x, expected %x\n",
> > -			       (ret & PR_MTE_TCF_MASK), mask);
> > +			       (ret & (int)PR_MTE_TCF_MASK), mask);
> >  		ksft_test_result_fail("%s\n", name);  
> 
> TBH my inclination is that this is worse than letting the value be
> promoted, casts (particularly casts of constants) are just obviously
> suspect in a way that printf() formats aren't.

Fair enough, I wasn't sure about this either, and indeed this down-cast of
a constant smells dodgy. So shall I just use %lx, and rely on the
promotion (so the MASK being defined as UL), or cast "ret" to long?

Cheers,
Andre
Mark Brown Aug. 16, 2024, 5:07 p.m. UTC | #3
On Fri, Aug 16, 2024 at 05:55:48PM +0100, Andre Przywara wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Aug 16, 2024 at 04:32:48PM +0100, Andre Przywara wrote:

> > >  		ksft_print_msg("Got %x, expected %x\n",
> > > -			       (ret & PR_MTE_TCF_MASK), mask);
> > > +			       (ret & (int)PR_MTE_TCF_MASK), mask);

> > TBH my inclination is that this is worse than letting the value be
> > promoted, casts (particularly casts of constants) are just obviously
> > suspect in a way that printf() formats aren't.

> Fair enough, I wasn't sure about this either, and indeed this down-cast of
> a constant smells dodgy. So shall I just use %lx, and rely on the
> promotion (so the MASK being defined as UL), or cast "ret" to long?

My inclination would be to go with the former.
diff mbox series

Patch

diff --git a/tools/testing/selftests/arm64/mte/check_prctl.c b/tools/testing/selftests/arm64/mte/check_prctl.c
index f139a33a43ef4..a16d7005affdf 100644
--- a/tools/testing/selftests/arm64/mte/check_prctl.c
+++ b/tools/testing/selftests/arm64/mte/check_prctl.c
@@ -81,11 +81,11 @@  void set_mode_test(const char *name, int hwcap2, int mask)
 		return;
 	}
 
-	if ((ret & PR_MTE_TCF_MASK) == mask) {
+	if ((ret & (int)PR_MTE_TCF_MASK) == mask) {
 		ksft_test_result_pass("%s\n", name);
 	} else {
 		ksft_print_msg("Got %x, expected %x\n",
-			       (ret & PR_MTE_TCF_MASK), mask);
+			       (ret & (int)PR_MTE_TCF_MASK), mask);
 		ksft_test_result_fail("%s\n", name);
 	}
 }