[PATCH] selinux: use native iterator types

[PATCH] selinux: use native iterator types

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Use types for iterators equal to the type of the to be compared values.

Reported by clang:

    security/selinux/ss/sidtab.c:126:2: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
      126 |         hash_for_each_rcu(sidtab->context_to_sid, i, entry, list) {
          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    ./include/linux/hashtable.h:139:51: note: expanded from macro 'hash_for_each_rcu'
      139 |         for ((bkt) = 0, obj = NULL; obj == NULL && (bkt) < HASH_SIZE(name);\
          |                                                     ~~~  ^ ~~~~~~~~~~~~~~~

    security/selinux/selinuxfs.c:1520:23: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
     1520 |         for (cpu = *idx; cpu < nr_cpu_ids; ++cpu) {
          |                          ~~~ ^ ~~~~~~~~~~

    security/selinux/hooks.c:412:16: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
      412 |         for (i = 0; i < ARRAY_SIZE(tokens); i++) {
          |                     ~ ^ ~~~~~~~~~~~~~~~~~~

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/hooks.c     | 2 +-
 security/selinux/selinuxfs.c | 2 +-
 security/selinux/ss/sidtab.c | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ad3abd48eed1..8cab0413df95 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -407,7 +407,7 @@ static const struct {
 
 static int match_opt_prefix(char *s, int l, char **arg)
 {
-	int i;
+	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(tokens); i++) {
 		size_t len = tokens[i].len;
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 234f4789b787..ea563e6215a1 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1515,7 +1515,7 @@ static const struct file_operations sel_avc_hash_stats_ops = {
 #ifdef CONFIG_SECURITY_SELINUX_AVC_STATS
 static struct avc_cache_stats *sel_avc_get_stat_idx(loff_t *idx)
 {
-	int cpu;
+	loff_t cpu;
 
 	for (cpu = *idx; cpu < nr_cpu_ids; ++cpu) {
 		if (!cpu_possible(cpu))
diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index c8848cbba81f..cb7125cc7f8e 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -114,12 +114,12 @@ int sidtab_set_initial(struct sidtab *s, u32 sid, struct context *context)
 
 int sidtab_hash_stats(struct sidtab *sidtab, char *page)
 {
-	int i;
+	unsigned int i;
 	int chain_len = 0;
 	int slots_used = 0;
 	int entries = 0;
 	int max_chain_len = 0;
-	int cur_bucket = 0;
+	unsigned int cur_bucket = 0;
 	struct sidtab_entry *entry;
 
 	rcu_read_lock();
-- 
2.45.2

Re: [PATCH] selinux: use native iterator types

[Paul Moore]

On Nov 25, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> Use types for iterators equal to the type of the to be compared values.
> 
> Reported by clang:
> 
>     security/selinux/ss/sidtab.c:126:2: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
>       126 |         hash_for_each_rcu(sidtab->context_to_sid, i, entry, list) {
>           |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     ./include/linux/hashtable.h:139:51: note: expanded from macro 'hash_for_each_rcu'
>       139 |         for ((bkt) = 0, obj = NULL; obj == NULL && (bkt) < HASH_SIZE(name);\
>           |                                                     ~~~  ^ ~~~~~~~~~~~~~~~
> 
>     security/selinux/selinuxfs.c:1520:23: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
>      1520 |         for (cpu = *idx; cpu < nr_cpu_ids; ++cpu) {
>           |                          ~~~ ^ ~~~~~~~~~~
> 
>     security/selinux/hooks.c:412:16: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
>       412 |         for (i = 0; i < ARRAY_SIZE(tokens); i++) {
>           |                     ~ ^ ~~~~~~~~~~~~~~~~~~
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/hooks.c     | 2 +-
>  security/selinux/selinuxfs.c | 2 +-
>  security/selinux/ss/sidtab.c | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)

Merged into selinux/dev, thanks Christian.

--
paul-moore.com

RE: [PATCH] selinux: use native iterator types

[David Laight]

From: Paul Moore
> Sent: 11 December 2024 18:54
> 
> On Nov 25, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> >
> > Use types for iterators equal to the type of the to be compared values.
> >
> > Reported by clang:
> >
> >     security/selinux/ss/sidtab.c:126:2: warning: comparison of integers of different signs: 'int'
> and 'unsigned long' [-Wsign-compare]
> >       126 |         hash_for_each_rcu(sidtab->context_to_sid, i, entry, list) {
> >           |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >     ./include/linux/hashtable.h:139:51: note: expanded from macro 'hash_for_each_rcu'
> >       139 |         for ((bkt) = 0, obj = NULL; obj == NULL && (bkt) < HASH_SIZE(name);\
> >           |                                                     ~~~  ^ ~~~~~~~~~~~~~~~
> >
> >     security/selinux/selinuxfs.c:1520:23: warning: comparison of integers of different signs: 'int'
> and 'unsigned int' [-Wsign-compare]
> >      1520 |         for (cpu = *idx; cpu < nr_cpu_ids; ++cpu) {
> >           |                          ~~~ ^ ~~~~~~~~~~
> >
> >     security/selinux/hooks.c:412:16: warning: comparison of integers of different signs: 'int' and
> 'unsigned long' [-Wsign-compare]
> >       412 |         for (i = 0; i < ARRAY_SIZE(tokens); i++) {
> >           |                     ~ ^ ~~~~~~~~~~~~~~~~~~

Isn't this an example of why -Wsign-compare is entirely stupid and isn't enabled
in the normal kernel build?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] selinux: use native iterator types

[Linus Torvalds]

On Sat, 14 Dec 2024 at 13:08, David Laight <David.Laight@aculab.com> wrote:
>
> Isn't this an example of why -Wsign-compare is entirely stupid and isn't enabled
> in the normal kernel build?

Yes. Please don't try to "fix" the warnings pointed out by that
completely broken warning.

I don't understand why some people seem to think "more warnings good".

-Wsign-compare is actively detrimental, and causes people to write
worse code (and often causes mindless type conversions that then
result in actual real bugs).

            Linus

Re: [PATCH] selinux: use native iterator types

[Paul Moore]

On Sat, Dec 14, 2024 at 4:10 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, 14 Dec 2024 at 13:08, David Laight <David.Laight@aculab.com> wrote:
> >
> > Isn't this an example of why -Wsign-compare is entirely stupid and isn't enabled
> > in the normal kernel build?
>
> Yes. Please don't try to "fix" the warnings pointed out by that
> completely broken warning.
>
> I don't understand why some people seem to think "more warnings good".
>
> -Wsign-compare is actively detrimental, and causes people to write
> worse code (and often causes mindless type conversions that then
> result in actual real bugs).

I'm not going to argue the usefulness of '-Wsign-compare' in a general
sense, but I will say that in my opinion the changes proposed by
Christian in this patch are correct and an improvement which is why I
merged the code.  If anyone has an objection to this particular path,
especially if you believe this introduces a bug, please let us know.

-- 
paul-moore.com

Re: [PATCH] selinux: use native iterator types

[Christian Göttsche]

On Sat, 14 Dec 2024 at 23:48, Paul Moore <paul@paul-moore.com> wrote:
>
> On Sat, Dec 14, 2024 at 4:10 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Sat, 14 Dec 2024 at 13:08, David Laight <David.Laight@aculab.com> wrote:
> > >
> > > Isn't this an example of why -Wsign-compare is entirely stupid and isn't enabled
> > > in the normal kernel build?
> >
> > Yes. Please don't try to "fix" the warnings pointed out by that
> > completely broken warning.
> >
> > I don't understand why some people seem to think "more warnings good".
> >
> > -Wsign-compare is actively detrimental, and causes people to write
> > worse code (and often causes mindless type conversions that then
> > result in actual real bugs).

I somehow like compiler warnings since for me they offload some trains
of thought and let me concentrate more an the overall code structure.
Also while not building the kernel with this warning, I like to enable
it in the language server in my editor.
And sometimes a warning can point at a real issue, see a65d9d1d893b
("ima: uncover hidden variable in ima_match_rules()").

> I'm not going to argue the usefulness of '-Wsign-compare' in a general
> sense, but I will say that in my opinion the changes proposed by
> Christian in this patch are correct and an improvement which is why I
> merged the code.  If anyone has an objection to this particular path,
> especially if you believe this introduces a bug, please let us know.
>
> --
> paul-moore.com