[PATCH v3 2/7] selinux: use u32 as bit type in ebitmap code

[PATCH v3 2/7] selinux: use u32 as bit type in ebitmap code

[Christian Göttsche]

The extensible bitmap supports bit positions up to U32_MAX due to the
type of the member highbit being u32.  Use u32 consistently as the type
for bit positions to announce to callers what range of values is
supported.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v3:
  - revert type change of unrelated iter variable
  - use U32_MAX instead of (u32)-1
v2: avoid declarations in init-clauses of for loops
---
 security/selinux/ss/ebitmap.c | 29 +++++++++++++++--------------
 security/selinux/ss/ebitmap.h | 32 ++++++++++++++++----------------
 2 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index 77875ad355f7..a313e633aa8e 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -24,7 +24,7 @@
 #include "ebitmap.h"
 #include "policydb.h"
 
-#define BITS_PER_U64	(sizeof(u64) * 8)
+#define BITS_PER_U64	((u32)(sizeof(u64) * 8))
 
 static struct kmem_cache *ebitmap_node_cachep __ro_after_init;
 
@@ -82,7 +82,8 @@ int ebitmap_cpy(struct ebitmap *dst, const struct ebitmap *src)
 int ebitmap_and(struct ebitmap *dst, const struct ebitmap *e1, const struct ebitmap *e2)
 {
 	struct ebitmap_node *n;
-	int bit, rc;
+	u32 bit;
+	int rc;
 
 	ebitmap_init(dst);
 
@@ -259,7 +260,7 @@ int ebitmap_contains(const struct ebitmap *e1, const struct ebitmap *e2, u32 las
 	return 1;
 }
 
-int ebitmap_get_bit(const struct ebitmap *e, unsigned long bit)
+int ebitmap_get_bit(const struct ebitmap *e, u32 bit)
 {
 	const struct ebitmap_node *n;
 
@@ -276,7 +277,7 @@ int ebitmap_get_bit(const struct ebitmap *e, unsigned long bit)
 	return 0;
 }
 
-int ebitmap_set_bit(struct ebitmap *e, unsigned long bit, int value)
+int ebitmap_set_bit(struct ebitmap *e, u32 bit, int value)
 {
 	struct ebitmap_node *n, *prev, *new;
 
@@ -287,7 +288,7 @@ int ebitmap_set_bit(struct ebitmap *e, unsigned long bit, int value)
 			if (value) {
 				ebitmap_node_set_bit(n, bit);
 			} else {
-				unsigned int s;
+				u32 s;
 
 				ebitmap_node_clr_bit(n, bit);
 
@@ -365,12 +366,12 @@ void ebitmap_destroy(struct ebitmap *e)
 int ebitmap_read(struct ebitmap *e, void *fp)
 {
 	struct ebitmap_node *n = NULL;
-	u32 mapunit, count, startbit, index;
+	u32 mapunit, count, startbit, index, i;
 	__le32 ebitmap_start;
 	u64 map;
 	__le64 mapbits;
 	__le32 buf[3];
-	int rc, i;
+	int rc;
 
 	ebitmap_init(e);
 
@@ -384,7 +385,7 @@ int ebitmap_read(struct ebitmap *e, void *fp)
 
 	if (mapunit != BITS_PER_U64) {
 		pr_err("SELinux: ebitmap: map size %u does not "
-		       "match my size %zd (high bit was %d)\n",
+		       "match my size %d (high bit was %d)\n",
 		       mapunit, BITS_PER_U64, e->highbit);
 		goto bad;
 	}
@@ -471,18 +472,18 @@ int ebitmap_read(struct ebitmap *e, void *fp)
 int ebitmap_write(const struct ebitmap *e, void *fp)
 {
 	struct ebitmap_node *n;
-	u32 count;
+	u32 bit, count, last_bit, last_startbit;
 	__le32 buf[3];
 	u64 map;
-	int bit, last_bit, last_startbit, rc;
+	int rc;
 
 	buf[0] = cpu_to_le32(BITS_PER_U64);
 
 	count = 0;
 	last_bit = 0;
-	last_startbit = -1;
+	last_startbit = U32_MAX;
 	ebitmap_for_each_positive_bit(e, n, bit) {
-		if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
+		if (last_startbit == U32_MAX || rounddown(bit, BITS_PER_U64) > last_startbit) {
 			count++;
 			last_startbit = rounddown(bit, BITS_PER_U64);
 		}
@@ -496,9 +497,9 @@ int ebitmap_write(const struct ebitmap *e, void *fp)
 		return rc;
 
 	map = 0;
-	last_startbit = INT_MIN;
+	last_startbit = U32_MAX;
 	ebitmap_for_each_positive_bit(e, n, bit) {
-		if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
+		if (last_startbit == U32_MAX || rounddown(bit, BITS_PER_U64) > last_startbit) {
 			__le64 buf64[1];
 
 			/* this is the very first bit */
diff --git a/security/selinux/ss/ebitmap.h b/security/selinux/ss/ebitmap.h
index e3c807cfad90..43c32077d483 100644
--- a/security/selinux/ss/ebitmap.h
+++ b/security/selinux/ss/ebitmap.h
@@ -44,10 +44,10 @@ struct ebitmap {
 
 #define ebitmap_length(e) ((e)->highbit)
 
-static inline unsigned int ebitmap_start_positive(const struct ebitmap *e,
+static inline u32 ebitmap_start_positive(const struct ebitmap *e,
 						  struct ebitmap_node **n)
 {
-	unsigned int ofs;
+	u32 ofs;
 
 	for (*n = e->node; *n; *n = (*n)->next) {
 		ofs = find_first_bit((*n)->maps, EBITMAP_SIZE);
@@ -62,11 +62,11 @@ static inline void ebitmap_init(struct ebitmap *e)
 	memset(e, 0, sizeof(*e));
 }
 
-static inline unsigned int ebitmap_next_positive(const struct ebitmap *e,
+static inline u32 ebitmap_next_positive(const struct ebitmap *e,
 						 struct ebitmap_node **n,
-						 unsigned int bit)
+						 u32 bit)
 {
-	unsigned int ofs;
+	u32 ofs;
 
 	ofs = find_next_bit((*n)->maps, EBITMAP_SIZE, bit - (*n)->startbit + 1);
 	if (ofs < EBITMAP_SIZE)
@@ -86,10 +86,10 @@ static inline unsigned int ebitmap_next_positive(const struct ebitmap *e,
 	(((bit) - (node)->startbit) % EBITMAP_UNIT_SIZE)
 
 static inline int ebitmap_node_get_bit(const struct ebitmap_node *n,
-				       unsigned int bit)
+				       u32 bit)
 {
-	unsigned int index = EBITMAP_NODE_INDEX(n, bit);
-	unsigned int ofs = EBITMAP_NODE_OFFSET(n, bit);
+	u32 index = EBITMAP_NODE_INDEX(n, bit);
+	u32 ofs = EBITMAP_NODE_OFFSET(n, bit);
 
 	BUG_ON(index >= EBITMAP_UNIT_NUMS);
 	if ((n->maps[index] & (EBITMAP_BIT << ofs)))
@@ -98,20 +98,20 @@ static inline int ebitmap_node_get_bit(const struct ebitmap_node *n,
 }
 
 static inline void ebitmap_node_set_bit(struct ebitmap_node *n,
-					unsigned int bit)
+					u32 bit)
 {
-	unsigned int index = EBITMAP_NODE_INDEX(n, bit);
-	unsigned int ofs = EBITMAP_NODE_OFFSET(n, bit);
+	u32 index = EBITMAP_NODE_INDEX(n, bit);
+	u32 ofs = EBITMAP_NODE_OFFSET(n, bit);
 
 	BUG_ON(index >= EBITMAP_UNIT_NUMS);
 	n->maps[index] |= (EBITMAP_BIT << ofs);
 }
 
 static inline void ebitmap_node_clr_bit(struct ebitmap_node *n,
-					unsigned int bit)
+					u32 bit)
 {
-	unsigned int index = EBITMAP_NODE_INDEX(n, bit);
-	unsigned int ofs = EBITMAP_NODE_OFFSET(n, bit);
+	u32 index = EBITMAP_NODE_INDEX(n, bit);
+	u32 ofs = EBITMAP_NODE_OFFSET(n, bit);
 
 	BUG_ON(index >= EBITMAP_UNIT_NUMS);
 	n->maps[index] &= ~(EBITMAP_BIT << ofs);
@@ -126,8 +126,8 @@ int ebitmap_cmp(const struct ebitmap *e1, const struct ebitmap *e2);
 int ebitmap_cpy(struct ebitmap *dst, const struct ebitmap *src);
 int ebitmap_and(struct ebitmap *dst, const struct ebitmap *e1, const struct ebitmap *e2);
 int ebitmap_contains(const struct ebitmap *e1, const struct ebitmap *e2, u32 last_e2bit);
-int ebitmap_get_bit(const struct ebitmap *e, unsigned long bit);
-int ebitmap_set_bit(struct ebitmap *e, unsigned long bit, int value);
+int ebitmap_get_bit(const struct ebitmap *e, u32 bit);
+int ebitmap_set_bit(struct ebitmap *e, u32 bit, int value);
 void ebitmap_destroy(struct ebitmap *e);
 int ebitmap_read(struct ebitmap *e, void *fp);
 int ebitmap_write(const struct ebitmap *e, void *fp);
-- 
2.40.1

[PATCH v3 3/7] selinux: update type for number of class permissions in services code

[Christian Göttsche]

Security classes have only up to 32 permissions, hence using an u16 is
sufficient (while improving padding in struct selinux_mapping).

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v3:
  - drop type change of arithmetic variable; it might effect performance
    as suggested by David.
  - split bogus and corrected cast into separate patch
v2:
   update commit description:
     - mention struct selinux_mapping  in the padding argument
       (currently between the first and second member there are 2 bytes
        padding)
     - mention overflow in the cast argument and the result of setting
       no bits due to it
---
 security/selinux/ss/services.c | 2 +-
 security/selinux/ss/services.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 3ec0bb39c234..dacec2ebdcd7 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -97,7 +97,6 @@ static int selinux_set_mapping(struct policydb *pol,
 			       struct selinux_map *out_map)
 {
 	u16 i, j;
-	unsigned k;
 	bool print_unknown_handle = false;
 
 	/* Find number of classes in the input mapping */
@@ -117,6 +116,7 @@ static int selinux_set_mapping(struct policydb *pol,
 	while (map[j].name) {
 		const struct security_class_mapping *p_in = map + (j++);
 		struct selinux_mapping *p_out = out_map->mapping + j;
+		u16 k;
 
 		/* An empty class string skips ahead */
 		if (!strcmp(p_in->name, "")) {
diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h
index ed2ee6600467..d24b0a3d198e 100644
--- a/security/selinux/ss/services.h
+++ b/security/selinux/ss/services.h
@@ -12,7 +12,7 @@
 /* Mapping for a single class */
 struct selinux_mapping {
 	u16 value; /* policy value for class */
-	unsigned int num_perms; /* number of permissions in class */
+	u16 num_perms; /* number of permissions in class */
 	u32 perms[sizeof(u32) * 8]; /* policy values for permissions */
 };
 
-- 
2.40.1

[PATCH v3 4/7] selinux: make left shifts well defined

[Christian Göttsche]

The loops upper bound represent the number of permissions used (for the
current class or in general).  The limit for this is 32, thus we might
left shift of one less, 31.  Shifting a base of 1 results in undefined
behavior; use (u32)1 as base.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v3: split from parent commit and apply cast to correct shift operand
---
 security/selinux/ss/services.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index dacec2ebdcd7..1eeffc66ea7d 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -207,22 +207,22 @@ static void map_decision(struct selinux_map *map,
 
 		for (i = 0, result = 0; i < n; i++) {
 			if (avd->allowed & mapping->perms[i])
-				result |= 1<<i;
+				result |= (u32)1<<i;
 			if (allow_unknown && !mapping->perms[i])
-				result |= 1<<i;
+				result |= (u32)1<<i;
 		}
 		avd->allowed = result;
 
 		for (i = 0, result = 0; i < n; i++)
 			if (avd->auditallow & mapping->perms[i])
-				result |= 1<<i;
+				result |= (u32)1<<i;
 		avd->auditallow = result;
 
 		for (i = 0, result = 0; i < n; i++) {
 			if (avd->auditdeny & mapping->perms[i])
-				result |= 1<<i;
+				result |= (u32)1<<i;
 			if (!allow_unknown && !mapping->perms[i])
-				result |= 1<<i;
+				result |= (u32)1<<i;
 		}
 		/*
 		 * In case the kernel has a bug and requests a permission
@@ -230,7 +230,7 @@ static void map_decision(struct selinux_map *map,
 		 * should audit that denial
 		 */
 		for (; i < (sizeof(u32)*8); i++)
-			result |= 1<<i;
+			result |= (u32)1<<i;
 		avd->auditdeny = result;
 	}
 }
-- 
2.40.1

[PATCH v3 5/7] selinux: avoid implicit conversions in selinuxfs code

[Christian Göttsche]

Use umode_t as parameter type for sel_make_inode(), which assigns the
value to the member i_mode of struct inode.

Use identical and unsigned types for loop iterators.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v3:
  - drop leftover declaration in init-clauses of for loops
  - use unsigned int instead of u32 for loop iterator with loop bounds
    known at compile time to be small (<100)
v2: avoid declarations in init-clauses of for loops
---
 security/selinux/selinuxfs.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index b969e87fd870..107b028d5e40 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -97,7 +97,7 @@ static int selinux_fs_info_create(struct super_block *sb)
 static void selinux_fs_info_free(struct super_block *sb)
 {
 	struct selinux_fs_info *fsi = sb->s_fs_info;
-	int i;
+	unsigned int i;
 
 	if (fsi) {
 		for (i = 0; i < fsi->bool_num; i++)
@@ -1075,8 +1075,8 @@ static ssize_t sel_write_user(struct file *file, char *buf, size_t size)
 	u32 sid, *sids = NULL;
 	ssize_t length;
 	char *newcon;
-	int i, rc;
-	u32 len, nsids;
+	int rc;
+	u32 i, len, nsids;
 
 	length = avc_has_perm(current_sid(), SECINITSID_SECURITY,
 			      SECCLASS_SECURITY, SECURITY__COMPUTE_USER,
@@ -1192,7 +1192,7 @@ static ssize_t sel_write_member(struct file *file, char *buf, size_t size)
 	return length;
 }
 
-static struct inode *sel_make_inode(struct super_block *sb, int mode)
+static struct inode *sel_make_inode(struct super_block *sb, umode_t mode)
 {
 	struct inode *ret = new_inode(sb);
 
@@ -1613,7 +1613,7 @@ static int sel_make_avc_files(struct dentry *dir)
 {
 	struct super_block *sb = dir->d_sb;
 	struct selinux_fs_info *fsi = sb->s_fs_info;
-	int i;
+	unsigned int i;
 	static const struct tree_descr files[] = {
 		{ "cache_threshold",
 		  &sel_avc_cache_threshold_ops, S_IRUGO|S_IWUSR },
@@ -1649,7 +1649,7 @@ static int sel_make_ss_files(struct dentry *dir)
 {
 	struct super_block *sb = dir->d_sb;
 	struct selinux_fs_info *fsi = sb->s_fs_info;
-	int i;
+	unsigned int i;
 	static const struct tree_descr files[] = {
 		{ "sidtab_hash_stats", &sel_sidtab_hash_stats_ops, S_IRUGO },
 	};
@@ -1700,7 +1700,7 @@ static const struct file_operations sel_initcon_ops = {
 
 static int sel_make_initcon_files(struct dentry *dir)
 {
-	int i;
+	unsigned int i;
 
 	for (i = 1; i <= SECINITSID_NUM; i++) {
 		struct inode *inode;
-- 
2.40.1

[PATCH v3 6/7] selinux: avoid implicit conversions in policydb code

[Christian Göttsche]

Use the identical type for local variables, e.g. loop counters.

Declare members of struct policydb_compat_info unsigned to consistently
use unsigned iterators.  They hold read-only non-negative numbers in the
global variable policydb_compat.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v3:
  - use unsigned int instead of u32 for iterators where the loop bound
    is known at compile time and small (<100)
    /@Paul: keep u32 iterator in policydb_destroy() due to
    /  for (i = 0; i < p->p_types.nprim; i++)
    /
  - drop not mentioned protocol and port checks regarding out of range
    values; there are a couple more of them and those changes are
    suitable for a different patchset
v2:
  - avoid declarations in init-clauses of for loops
  - declare members of struct policydb_compat_info unsigned
---
 security/selinux/ss/policydb.c | 69 ++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 32 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index a424997c79eb..c3ffe78ef144 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -55,9 +55,9 @@ static const char *const symtab_name[SYM_NUM] = {
 #endif
 
 struct policydb_compat_info {
-	int version;
-	int sym_num;
-	int ocon_num;
+	unsigned int version;
+	unsigned int sym_num;
+	unsigned int ocon_num;
 };
 
 /* These need to be updated if SYM_NUM or OCON_NUM changes */
@@ -159,9 +159,9 @@ static const struct policydb_compat_info policydb_compat[] = {
 	},
 };
 
-static const struct policydb_compat_info *policydb_lookup_compat(int version)
+static const struct policydb_compat_info *policydb_lookup_compat(unsigned int version)
 {
-	int i;
+	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(policydb_compat); i++) {
 		if (policydb_compat[i].version == version)
@@ -359,7 +359,7 @@ static int role_tr_destroy(void *key, void *datum, void *p)
 	return 0;
 }
 
-static void ocontext_destroy(struct ocontext *c, int i)
+static void ocontext_destroy(struct ocontext *c, unsigned int i)
 {
 	if (!c)
 		return;
@@ -782,7 +782,7 @@ void policydb_destroy(struct policydb *p)
 {
 	struct ocontext *c, *ctmp;
 	struct genfs *g, *gtmp;
-	int i;
+	u32 i;
 	struct role_allow *ra, *lra = NULL;
 
 	for (i = 0; i < SYM_NUM; i++) {
@@ -1155,8 +1155,8 @@ static int common_read(struct policydb *p, struct symtab *s, void *fp)
 	char *key = NULL;
 	struct common_datum *comdatum;
 	__le32 buf[4];
-	u32 len, nel;
-	int i, rc;
+	u32 i, len, nel;
+	int rc;
 
 	comdatum = kzalloc(sizeof(*comdatum), GFP_KERNEL);
 	if (!comdatum)
@@ -1221,13 +1221,13 @@ static int type_set_read(struct type_set *t, void *fp)
 
 static int read_cons_helper(struct policydb *p,
 				struct constraint_node **nodep,
-				int ncons, int allowxtarget, void *fp)
+				u32 ncons, int allowxtarget, void *fp)
 {
 	struct constraint_node *c, *lc;
 	struct constraint_expr *e, *le;
 	__le32 buf[3];
-	u32 nexpr;
-	int rc, i, j, depth;
+	u32 i, j, nexpr;
+	int rc, depth;
 
 	lc = NULL;
 	for (i = 0; i < ncons; i++) {
@@ -1319,8 +1319,8 @@ static int class_read(struct policydb *p, struct symtab *s, void *fp)
 	char *key = NULL;
 	struct class_datum *cladatum;
 	__le32 buf[6];
-	u32 len, len2, ncons, nel;
-	int i, rc;
+	u32 i, len, len2, ncons, nel;
+	int rc;
 
 	cladatum = kzalloc(sizeof(*cladatum), GFP_KERNEL);
 	if (!cladatum)
@@ -1413,7 +1413,8 @@ static int role_read(struct policydb *p, struct symtab *s, void *fp)
 {
 	char *key = NULL;
 	struct role_datum *role;
-	int rc, to_read = 2;
+	int rc;
+	unsigned int to_read = 2;
 	__le32 buf[3];
 	u32 len;
 
@@ -1469,7 +1470,8 @@ static int type_read(struct policydb *p, struct symtab *s, void *fp)
 {
 	char *key = NULL;
 	struct type_datum *typdatum;
-	int rc, to_read = 3;
+	int rc;
+	unsigned int to_read = 3;
 	__le32 buf[4];
 	u32 len;
 
@@ -1543,7 +1545,8 @@ static int user_read(struct policydb *p, struct symtab *s, void *fp)
 {
 	char *key = NULL;
 	struct user_datum *usrdatum;
-	int rc, to_read = 2;
+	int rc;
+	unsigned int to_read = 2;
 	__le32 buf[3];
 	u32 len;
 
@@ -1684,7 +1687,7 @@ static int user_bounds_sanity_check(void *key, void *datum, void *datap)
 	upper = user = datum;
 	while (upper->bounds) {
 		struct ebitmap_node *node;
-		unsigned long bit;
+		u32 bit;
 
 		if (++depth == POLICYDB_BOUNDS_MAXDEPTH) {
 			pr_err("SELinux: user %s: "
@@ -1720,7 +1723,7 @@ static int role_bounds_sanity_check(void *key, void *datum, void *datap)
 	upper = role = datum;
 	while (upper->bounds) {
 		struct ebitmap_node *node;
-		unsigned long bit;
+		u32 bit;
 
 		if (++depth == POLICYDB_BOUNDS_MAXDEPTH) {
 			pr_err("SELinux: role %s: "
@@ -1835,9 +1838,9 @@ static int range_read(struct policydb *p, void *fp)
 {
 	struct range_trans *rt = NULL;
 	struct mls_range *r = NULL;
-	int i, rc;
+	int rc;
 	__le32 buf[2];
-	u32 nel;
+	u32 i, nel;
 
 	if (p->policyvers < POLICYDB_VERSION_MLS)
 		return 0;
@@ -2083,9 +2086,9 @@ static int filename_trans_read_helper(struct policydb *p, void *fp)
 
 static int filename_trans_read(struct policydb *p, void *fp)
 {
-	u32 nel;
+	u32 nel, i;
 	__le32 buf[1];
-	int rc, i;
+	int rc;
 
 	if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
 		return 0;
@@ -2124,8 +2127,8 @@ static int filename_trans_read(struct policydb *p, void *fp)
 
 static int genfs_read(struct policydb *p, void *fp)
 {
-	int i, j, rc;
-	u32 nel, nel2, len, len2;
+	int rc;
+	u32 i, j, nel, nel2, len, len2;
 	__le32 buf[1];
 	struct ocontext *l, *c;
 	struct ocontext *newc = NULL;
@@ -2238,8 +2241,9 @@ static int genfs_read(struct policydb *p, void *fp)
 static int ocontext_read(struct policydb *p, const struct policydb_compat_info *info,
 			 void *fp)
 {
-	int i, j, rc;
-	u32 nel, len;
+	int rc;
+	unsigned int i;
+	u32 j, nel, len;
 	__be64 prefixbuf[1];
 	__le32 buf[3];
 	struct ocontext *l, *c;
@@ -2430,9 +2434,9 @@ int policydb_read(struct policydb *p, void *fp)
 	struct role_allow *ra, *lra;
 	struct role_trans_key *rtk = NULL;
 	struct role_trans_datum *rtd = NULL;
-	int i, j, rc;
+	int rc;
 	__le32 buf[4];
-	u32 len, nprim, nel, perm;
+	u32 i, j, len, nprim, nel, perm;
 
 	char *policydb_str;
 	const struct policydb_compat_info *info;
@@ -3283,7 +3287,8 @@ static int (*const write_f[SYM_NUM]) (void *key, void *datum, void *datap) = {
 static int ocontext_write(struct policydb *p, const struct policydb_compat_info *info,
 			  void *fp)
 {
-	unsigned int i, j, rc;
+	unsigned int i, j;
+	int rc;
 	size_t nel, len;
 	__be64 prefixbuf[1];
 	__le32 buf[3];
@@ -3632,10 +3637,10 @@ static int filename_trans_write(struct policydb *p, void *fp)
  */
 int policydb_write(struct policydb *p, void *fp)
 {
-	unsigned int i, num_syms;
+	unsigned int num_syms;
 	int rc;
 	__le32 buf[4];
-	u32 config;
+	u32 config, i;
 	size_t len;
 	const struct policydb_compat_info *info;
 
-- 
2.40.1

[PATCH v3 7/7] selinux: use unsigned iterator in nlmsgtab code

[Christian Göttsche]

Use an unsigned type as loop iterator.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v3: use unsigned int instead of u32 since the loop bound is known at
    compile time and small (<100)
v2: avoid declarations in init-clauses of for loops
---
 security/selinux/nlmsgtab.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
index 2ee7b4ed43ef..8ff670cf1ee5 100644
--- a/security/selinux/nlmsgtab.c
+++ b/security/selinux/nlmsgtab.c
@@ -153,7 +153,8 @@ static const struct nlmsg_perm nlmsg_audit_perms[] = {
 
 static int nlmsg_perm(u16 nlmsg_type, u32 *perm, const struct nlmsg_perm *tab, size_t tabsize)
 {
-	int i, err = -EINVAL;
+	unsigned int i;
+	int err = -EINVAL;
 
 	for (i = 0; i < tabsize/sizeof(struct nlmsg_perm); i++)
 		if (nlmsg_type == tab[i].nlmsg_type) {
-- 
2.40.1

[PATCH v3 1/7] selinux: avoid implicit conversions in avtab code

[Christian Göttsche]

Return u32 from avtab_hash() instead of int, since the hashing is done
on u32 and the result is used as an index on the hash array.

Use the type of the limit in for loops.

Avoid signed to unsigned conversion of multiplication result in
avtab_hash_eval() and perform multiplication in destination type.

Use unsigned loop iterator for index operations, to avoid sign
extension.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v3:
  - use fixed sized counters in avtab_hash_eval()
  - perform multiplication in avtab_hash_eval() in destination type
v2: avoid declarations in init-clauses of for loops
---
 security/selinux/ss/avtab.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 243e5dabfa86..86d98a8e291b 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -29,7 +29,7 @@ static struct kmem_cache *avtab_xperms_cachep __ro_after_init;
 /* Based on MurmurHash3, written by Austin Appleby and placed in the
  * public domain.
  */
-static inline int avtab_hash(const struct avtab_key *keyp, u32 mask)
+static inline u32 avtab_hash(const struct avtab_key *keyp, u32 mask)
 {
 	static const u32 c1 = 0xcc9e2d51;
 	static const u32 c2 = 0x1b873593;
@@ -66,7 +66,7 @@ static inline int avtab_hash(const struct avtab_key *keyp, u32 mask)
 }
 
 static struct avtab_node*
-avtab_insert_node(struct avtab *h, int hvalue,
+avtab_insert_node(struct avtab *h, u32 hvalue,
 		  struct avtab_node *prev,
 		  const struct avtab_key *key, const struct avtab_datum *datum)
 {
@@ -106,7 +106,7 @@ avtab_insert_node(struct avtab *h, int hvalue,
 static int avtab_insert(struct avtab *h, const struct avtab_key *key,
 			const struct avtab_datum *datum)
 {
-	int hvalue;
+	u32 hvalue;
 	struct avtab_node *prev, *cur, *newnode;
 	u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
 
@@ -152,7 +152,7 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h,
 					  const struct avtab_key *key,
 					  const struct avtab_datum *datum)
 {
-	int hvalue;
+	u32 hvalue;
 	struct avtab_node *prev, *cur;
 	u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
 
@@ -186,7 +186,7 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h,
 struct avtab_node *avtab_search_node(struct avtab *h,
 				     const struct avtab_key *key)
 {
-	int hvalue;
+	u32 hvalue;
 	struct avtab_node *cur;
 	u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
 
@@ -246,7 +246,7 @@ avtab_search_node_next(struct avtab_node *node, u16 specified)
 
 void avtab_destroy(struct avtab *h)
 {
-	int i;
+	u32 i;
 	struct avtab_node *cur, *temp;
 
 	if (!h)
@@ -325,7 +325,7 @@ int avtab_alloc_dup(struct avtab *new, const struct avtab *orig)
 #ifdef CONFIG_SECURITY_SELINUX_DEBUG
 void avtab_hash_eval(struct avtab *h, const char *tag)
 {
-	int i, chain_len, slots_used, max_chain_len;
+	u32 i, chain_len, slots_used, max_chain_len;
 	unsigned long long chain2_len_sum;
 	struct avtab_node *cur;
 
@@ -344,7 +344,7 @@ void avtab_hash_eval(struct avtab *h, const char *tag)
 
 			if (chain_len > max_chain_len)
 				max_chain_len = chain_len;
-			chain2_len_sum += chain_len * chain_len;
+			chain2_len_sum += (unsigned long long)chain_len * chain_len;
 		}
 	}
 
@@ -374,13 +374,13 @@ int avtab_read_item(struct avtab *a, void *fp, struct policydb *pol,
 {
 	__le16 buf16[4];
 	u16 enabled;
-	u32 items, items2, val, vers = pol->policyvers;
+	u32 items, items2, val, i;
 	struct avtab_key key;
 	struct avtab_datum datum;
 	struct avtab_extended_perms xperms;
 	__le32 buf32[ARRAY_SIZE(xperms.perms.p)];
-	int i, rc;
-	unsigned set;
+	int rc;
+	unsigned int set, vers = pol->policyvers;
 
 	memset(&key, 0, sizeof(struct avtab_key));
 	memset(&datum, 0, sizeof(struct avtab_datum));
@@ -616,7 +616,7 @@ int avtab_write_item(struct policydb *p, const struct avtab_node *cur, void *fp)
 
 int avtab_write(struct policydb *p, struct avtab *a, void *fp)
 {
-	unsigned int i;
+	u32 i;
 	int rc = 0;
 	struct avtab_node *cur;
 	__le32 buf[1];
-- 
2.40.1

Re: [PATCH v3 1/7] selinux: avoid implicit conversions in avtab code

[Paul Moore]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

On Aug  7, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> wrote:
> 
> Return u32 from avtab_hash() instead of int, since the hashing is done
> on u32 and the result is used as an index on the hash array.
> 
> Use the type of the limit in for loops.
> 
> Avoid signed to unsigned conversion of multiplication result in
> avtab_hash_eval() and perform multiplication in destination type.
> 
> Use unsigned loop iterator for index operations, to avoid sign
> extension.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> v3:
>   - use fixed sized counters in avtab_hash_eval()
>   - perform multiplication in avtab_hash_eval() in destination type
> v2: avoid declarations in init-clauses of for loops
> ---
>  security/selinux/ss/avtab.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)

Merged into selinux/next, thanks.

--
paul-moore.com

Re: [PATCH v3 2/7] selinux: use u32 as bit type in ebitmap code

[Paul Moore]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3221 bytes --]

On Aug  7, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> wrote:
> 
> The extensible bitmap supports bit positions up to U32_MAX due to the
> type of the member highbit being u32.  Use u32 consistently as the type
> for bit positions to announce to callers what range of values is
> supported.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> v3:
>   - revert type change of unrelated iter variable
>   - use U32_MAX instead of (u32)-1
> v2: avoid declarations in init-clauses of for loops
> ---
>  security/selinux/ss/ebitmap.c | 29 +++++++++++++++--------------
>  security/selinux/ss/ebitmap.h | 32 ++++++++++++++++----------------
>  2 files changed, 31 insertions(+), 30 deletions(-)

...

> diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> index 77875ad355f7..a313e633aa8e 100644
> --- a/security/selinux/ss/ebitmap.c
> +++ b/security/selinux/ss/ebitmap.c
> @@ -471,18 +472,18 @@ int ebitmap_read(struct ebitmap *e, void *fp)
>  int ebitmap_write(const struct ebitmap *e, void *fp)
>  {
>  	struct ebitmap_node *n;
> -	u32 count;
> +	u32 bit, count, last_bit, last_startbit;
>  	__le32 buf[3];
>  	u64 map;
> -	int bit, last_bit, last_startbit, rc;
> +	int rc;
>  
>  	buf[0] = cpu_to_le32(BITS_PER_U64);
>  
>  	count = 0;
>  	last_bit = 0;
> -	last_startbit = -1;
> +	last_startbit = U32_MAX;
>  	ebitmap_for_each_positive_bit(e, n, bit) {
> -		if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
> +		if (last_startbit == U32_MAX || rounddown(bit, BITS_PER_U64) > last_startbit) {

I'm getting worried about what might happen if the ebitmap starts to
contain bits near the end of the range, e.g. U32_MAX.  When lastbit
was signed this was a non-issue as we could set it to a negative
value (-1) and not worry about it, although the maximum value
difference between the signed and unsigned types would eventually be
a problem.

While looking closer at this loop, I'm now wondering if we shouldn't
just rewrite the logic a bit to simplify things, and possibly speed
it up a small amount.  How about something like this:

  count = 1;
  n = e->node;
  while (n->next) {
    count++;
    n = n->next;
  }
  last_startbit = n->startbit;
  last_bit = n->startbit + find_last_bit(n->maps, EBITMAP_SIZE);

You should probably verify that there isn't something stupid like an
off-by-one bug in the code above, but I think it is a lot cleaner
than what we currently have and should resolve a lot of the type/math
issues.

>  			count++;
>  			last_startbit = rounddown(bit, BITS_PER_U64);
>  		}
> @@ -496,9 +497,9 @@ int ebitmap_write(const struct ebitmap *e, void *fp)
>  		return rc;
>  
>  	map = 0;
> -	last_startbit = INT_MIN;
> +	last_startbit = U32_MAX;
>  	ebitmap_for_each_positive_bit(e, n, bit) {
> -		if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
> +		if (last_startbit == U32_MAX || rounddown(bit, BITS_PER_U64) > last_startbit) {
>  			__le64 buf64[1];

Similar to the above, I think we can probably rewrite this to simply
walk the ebitmap nodes and write them out.  Using
ebitmap_for_each_positive_bit() seems overly complicated to me,
although I may be missing something important and obvious ...

--
paul-moore.com

Re: [PATCH v3 3/7] selinux: update type for number of class permissions in services code

[Paul Moore]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

On Aug  7, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> wrote:
> 
> Security classes have only up to 32 permissions, hence using an u16 is
> sufficient (while improving padding in struct selinux_mapping).
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> v3:
>   - drop type change of arithmetic variable; it might effect performance
>     as suggested by David.
>   - split bogus and corrected cast into separate patch
> v2:
>    update commit description:
>      - mention struct selinux_mapping  in the padding argument
>        (currently between the first and second member there are 2 bytes
>         padding)
>      - mention overflow in the cast argument and the result of setting
>        no bits due to it
> ---
>  security/selinux/ss/services.c | 2 +-
>  security/selinux/ss/services.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Merged into selinux/next, thanks.

--
paul-moore.com

Re: [PATCH v3 4/7] selinux: make left shifts well defined

[Paul Moore]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 646 bytes --]

On Aug  7, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> wrote:
> 
> The loops upper bound represent the number of permissions used (for the
> current class or in general).  The limit for this is 32, thus we might
> left shift of one less, 31.  Shifting a base of 1 results in undefined
> behavior; use (u32)1 as base.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> v3: split from parent commit and apply cast to correct shift operand
> ---
>  security/selinux/ss/services.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Merged into selinux/next, thanks.

--
paul-moore.com

Re: [PATCH v3 5/7] selinux: avoid implicit conversions in selinuxfs code

[Paul Moore]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On Aug  7, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> wrote:
> 
> Use umode_t as parameter type for sel_make_inode(), which assigns the
> value to the member i_mode of struct inode.
> 
> Use identical and unsigned types for loop iterators.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> v3:
>   - drop leftover declaration in init-clauses of for loops
>   - use unsigned int instead of u32 for loop iterator with loop bounds
>     known at compile time to be small (<100)
> v2: avoid declarations in init-clauses of for loops
> ---
>  security/selinux/selinuxfs.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

Merged into selinux/next, thanks.

--
paul-moore.com

Re: [PATCH v3 6/7] selinux: avoid implicit conversions in policydb code

[Paul Moore]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

On Aug  7, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> wrote:
> 
> Use the identical type for local variables, e.g. loop counters.
> 
> Declare members of struct policydb_compat_info unsigned to consistently
> use unsigned iterators.  They hold read-only non-negative numbers in the
> global variable policydb_compat.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> v3:
>   - use unsigned int instead of u32 for iterators where the loop bound
>     is known at compile time and small (<100)
>     /@Paul: keep u32 iterator in policydb_destroy() due to
>     /  for (i = 0; i < p->p_types.nprim; i++)
>     /
>   - drop not mentioned protocol and port checks regarding out of range
>     values; there are a couple more of them and those changes are
>     suitable for a different patchset
> v2:
>   - avoid declarations in init-clauses of for loops
>   - declare members of struct policydb_compat_info unsigned
> ---
>  security/selinux/ss/policydb.c | 69 ++++++++++++++++++----------------
>  1 file changed, 37 insertions(+), 32 deletions(-)

Merged into selinux/next, thanks.

--
paul-moore.com

Re: [PATCH v3 7/7] selinux: use unsigned iterator in nlmsgtab code

[Paul Moore]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On Aug  7, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> wrote:
> 
> Use an unsigned type as loop iterator.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> v3: use unsigned int instead of u32 since the loop bound is known at
>     compile time and small (<100)
> v2: avoid declarations in init-clauses of for loops
> ---
>  security/selinux/nlmsgtab.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Merged into selinux/next, thanks.

--
paul-moore.com

Re: [PATCH v3 2/7] selinux: use u32 as bit type in ebitmap code

[Christian Göttsche]

On Thu, 10 Aug 2023 at 01:07, Paul Moore <paul@paul-moore.com> wrote:
>
> On Aug  7, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> wrote:
> >
> > The extensible bitmap supports bit positions up to U32_MAX due to the
> > type of the member highbit being u32.  Use u32 consistently as the type
> > for bit positions to announce to callers what range of values is
> > supported.
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > ---
> > v3:
> >   - revert type change of unrelated iter variable
> >   - use U32_MAX instead of (u32)-1
> > v2: avoid declarations in init-clauses of for loops
> > ---
> >  security/selinux/ss/ebitmap.c | 29 +++++++++++++++--------------
> >  security/selinux/ss/ebitmap.h | 32 ++++++++++++++++----------------
> >  2 files changed, 31 insertions(+), 30 deletions(-)
>
> ...
>
> > diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> > index 77875ad355f7..a313e633aa8e 100644
> > --- a/security/selinux/ss/ebitmap.c
> > +++ b/security/selinux/ss/ebitmap.c
> > @@ -471,18 +472,18 @@ int ebitmap_read(struct ebitmap *e, void *fp)
> >  int ebitmap_write(const struct ebitmap *e, void *fp)
> >  {
> >       struct ebitmap_node *n;
> > -     u32 count;
> > +     u32 bit, count, last_bit, last_startbit;
> >       __le32 buf[3];
> >       u64 map;
> > -     int bit, last_bit, last_startbit, rc;
> > +     int rc;
> >
> >       buf[0] = cpu_to_le32(BITS_PER_U64);
> >
> >       count = 0;
> >       last_bit = 0;
> > -     last_startbit = -1;
> > +     last_startbit = U32_MAX;
> >       ebitmap_for_each_positive_bit(e, n, bit) {
> > -             if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
> > +             if (last_startbit == U32_MAX || rounddown(bit, BITS_PER_U64) > last_startbit) {
>
> I'm getting worried about what might happen if the ebitmap starts to
> contain bits near the end of the range, e.g. U32_MAX.  When lastbit
> was signed this was a non-issue as we could set it to a negative
> value (-1) and not worry about it, although the maximum value
> difference between the signed and unsigned types would eventually be
> a problem.

For the maximum bit of U32_MAX `rounddown(bit, BITS_PER_U64)` will
return U32_MAX-63, so it does not collide with the special
last_startbit value of U32_MAX.

> While looking closer at this loop, I'm now wondering if we shouldn't
> just rewrite the logic a bit to simplify things, and possibly speed
> it up a small amount.  How about something like this:
>
>   count = 1;
>   n = e->node;
>   while (n->next) {
>     count++;
>     n = n->next;
>   }
>   last_startbit = n->startbit;
>   last_bit = n->startbit + find_last_bit(n->maps, EBITMAP_SIZE);
>
> You should probably verify that there isn't something stupid like an
> off-by-one bug in the code above, but I think it is a lot cleaner
> than what we currently have and should resolve a lot of the type/math
> issues.

I think this loop does not work, since in the binary format the map
size is 64 bits (and thus we need to calculate the number of 64bit
nodes), but the kernel supports (depending on the architecture) 32bit
maps for the in-memory representation.
So the number of in-memory nodes might not be the same as the number
of nodes in binary format.

p.s.:

Looking at the patch again, `rounddown(bit, BITS_PER_U64)` is computed
twice and last_bit can probably be dropped in favor of e->highbit.

>
> >                       count++;
> >                       last_startbit = rounddown(bit, BITS_PER_U64);
> >               }
> > @@ -496,9 +497,9 @@ int ebitmap_write(const struct ebitmap *e, void *fp)
> >               return rc;
> >
> >       map = 0;
> > -     last_startbit = INT_MIN;
> > +     last_startbit = U32_MAX;
> >       ebitmap_for_each_positive_bit(e, n, bit) {
> > -             if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
> > +             if (last_startbit == U32_MAX || rounddown(bit, BITS_PER_U64) > last_startbit) {
> >                       __le64 buf64[1];
>
> Similar to the above, I think we can probably rewrite this to simply
> walk the ebitmap nodes and write them out.  Using
> ebitmap_for_each_positive_bit() seems overly complicated to me,
> although I may be missing something important and obvious ...
>
> --
> paul-moore.com

Re: [PATCH v3 2/7] selinux: use u32 as bit type in ebitmap code

[Christian Göttsche]

On Wed, 16 Aug 2023 at 17:00, Christian Göttsche <cgzones@googlemail.com> wrote:
>
> On Thu, 10 Aug 2023 at 01:07, Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Aug  7, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> wrote:
> > >
> > > The extensible bitmap supports bit positions up to U32_MAX due to the
> > > type of the member highbit being u32.  Use u32 consistently as the type
> > > for bit positions to announce to callers what range of values is
> > > supported.
> > >
> > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > > ---
> > > v3:
> > >   - revert type change of unrelated iter variable
> > >   - use U32_MAX instead of (u32)-1
> > > v2: avoid declarations in init-clauses of for loops
> > > ---
> > >  security/selinux/ss/ebitmap.c | 29 +++++++++++++++--------------
> > >  security/selinux/ss/ebitmap.h | 32 ++++++++++++++++----------------
> > >  2 files changed, 31 insertions(+), 30 deletions(-)
> >
> > ...
> >
> > > diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> > > index 77875ad355f7..a313e633aa8e 100644
> > > --- a/security/selinux/ss/ebitmap.c
> > > +++ b/security/selinux/ss/ebitmap.c
> > > @@ -471,18 +472,18 @@ int ebitmap_read(struct ebitmap *e, void *fp)
> > >  int ebitmap_write(const struct ebitmap *e, void *fp)
> > >  {
> > >       struct ebitmap_node *n;
> > > -     u32 count;
> > > +     u32 bit, count, last_bit, last_startbit;
> > >       __le32 buf[3];
> > >       u64 map;
> > > -     int bit, last_bit, last_startbit, rc;
> > > +     int rc;
> > >
> > >       buf[0] = cpu_to_le32(BITS_PER_U64);
> > >
> > >       count = 0;
> > >       last_bit = 0;
> > > -     last_startbit = -1;
> > > +     last_startbit = U32_MAX;
> > >       ebitmap_for_each_positive_bit(e, n, bit) {
> > > -             if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
> > > +             if (last_startbit == U32_MAX || rounddown(bit, BITS_PER_U64) > last_startbit) {
> >
> > I'm getting worried about what might happen if the ebitmap starts to
> > contain bits near the end of the range, e.g. U32_MAX.  When lastbit
> > was signed this was a non-issue as we could set it to a negative
> > value (-1) and not worry about it, although the maximum value
> > difference between the signed and unsigned types would eventually be
> > a problem.
>
> For the maximum bit of U32_MAX `rounddown(bit, BITS_PER_U64)` will
> return U32_MAX-63, so it does not collide with the special
> last_startbit value of U32_MAX.

Also the current implementation is not safe for bits in the range
[rounddown(U32_MAX, EBITMAP_SIZE), U32_MAX], since the highbit and
`startbit + EBITMAP_SIZE` calculations are not checked for overflows
(since EBITMAP_UNIT_SIZE is not a power of 2 (it's 6 on x64).

>
> > While looking closer at this loop, I'm now wondering if we shouldn't
> > just rewrite the logic a bit to simplify things, and possibly speed
> > it up a small amount.  How about something like this:
> >
> >   count = 1;
> >   n = e->node;
> >   while (n->next) {
> >     count++;
> >     n = n->next;
> >   }
> >   last_startbit = n->startbit;
> >   last_bit = n->startbit + find_last_bit(n->maps, EBITMAP_SIZE);
> >
> > You should probably verify that there isn't something stupid like an
> > off-by-one bug in the code above, but I think it is a lot cleaner
> > than what we currently have and should resolve a lot of the type/math
> > issues.
>
> I think this loop does not work, since in the binary format the map
> size is 64 bits (and thus we need to calculate the number of 64bit
> nodes), but the kernel supports (depending on the architecture) 32bit
> maps for the in-memory representation.
> So the number of in-memory nodes might not be the same as the number
> of nodes in binary format.
>
> p.s.:
>
> Looking at the patch again, `rounddown(bit, BITS_PER_U64)` is computed
> twice and last_bit can probably be dropped in favor of e->highbit.

The last_bit comment can be ignored, since last_bit is the highbit for
the mapsize of the binary format, so it's no equal to e->highbit
(which is relative to the in-memory mapsize).

>
> >
> > >                       count++;
> > >                       last_startbit = rounddown(bit, BITS_PER_U64);
> > >               }
> > > @@ -496,9 +497,9 @@ int ebitmap_write(const struct ebitmap *e, void *fp)
> > >               return rc;
> > >
> > >       map = 0;
> > > -     last_startbit = INT_MIN;
> > > +     last_startbit = U32_MAX;
> > >       ebitmap_for_each_positive_bit(e, n, bit) {
> > > -             if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
> > > +             if (last_startbit == U32_MAX || rounddown(bit, BITS_PER_U64) > last_startbit) {
> > >                       __le64 buf64[1];
> >
> > Similar to the above, I think we can probably rewrite this to simply
> > walk the ebitmap nodes and write them out.  Using
> > ebitmap_for_each_positive_bit() seems overly complicated to me,
> > although I may be missing something important and obvious ...
> >
> > --
> > paul-moore.com