Re: [PATCH] genetlink: fix unsigned int comparison with less than zero
On Sun, Nov 13, 2016 at 9:15 AM, David Miller wrote: > I've commited the following to net-next: > > > [PATCH] genetlink: Make family a signed integer. > > The idr_alloc(), idr_remove(), et al. routines all expect IDs to be > signed integers. Therefore make the genl_family member 'id' signed > too. This is exactly what I replied to Johannes. Thanks for the fix!
Re: [PATCH] genetlink: fix unsigned int comparison with less than zero
From: Colin King Date: Thu, 10 Nov 2016 15:57:58 + > From: Colin Ian King > > family->id is unsigned, so the less than zero check for > failure return from idr_alloc is never true and so the error exit > is never handled. Instead, assign err and check if this is less > than zero since this is a signed integer. > > Issue found with static analysis with CoverityScan, CID 1375916 > > Signed-off-by: Colin Ian King > --- > net/netlink/genetlink.c | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c > index f0b65fe..2ea61ba 100644 > --- a/net/netlink/genetlink.c > +++ b/net/netlink/genetlink.c > @@ -360,12 +360,10 @@ int genl_register_family(struct genl_family *family) > } else > family->attrbuf = NULL; > > - family->id = idr_alloc(&genl_fam_idr, family, > + family->id = err = idr_alloc(&genl_fam_idr, family, > start, end + 1, GFP_KERNEL); First of all, if we make this change you must fixup the indentation of the second l ine of this idr_alloc() call. Arguments spanning multiple lines of a call must be indented precisely to the column following the openning parenthesis of the first line. Next, the IDR helpers never give us values that fall within the positive range of an integer that does not fall within the postive range of an unsigned integer. We are going to pass this value in later to release the ID, again the interface will expect a signed rather than an unsigned int. Therefore is makes sesne only to change the family->id type to what it must be, which is a signed int. I've commited the following to net-next: [PATCH] genetlink: Make family a signed integer. The idr_alloc(), idr_remove(), et al. routines all expect IDs to be signed integers. Therefore make the genl_family member 'id' signed too. Signed-off-by: David S. Miller --- include/net/genetlink.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/net/genetlink.h b/include/net/genetlink.h index 3ec87ba..a34275b 100644 --- a/include/net/genetlink.h +++ b/include/net/genetlink.h @@ -48,7 +48,7 @@ struct genl_info; * @n_ops: number of operations supported by this family */ struct genl_family { - unsigned intid; /* private */ + int id; /* private */ unsigned inthdrsize; charname[GENL_NAMSIZ]; unsigned intversion; -- 2.7.4
Re: [PATCH] genetlink: fix unsigned int comparison with less than zero
> > I suppose it could be, since family IDs are allocated in a 16-bit > > range > > anyway. But family IDs can also never actually be negative, so > > having > > an unsigned int in the struct makes sense too. > > All idr_* API's accept int, rather than unsigned int. This is my > point. Sure, but that's an internal implementation detail. The struct genl_family is also an external API towards its users, and there negative numbers make no sense whatsoever. johannes
Re: [PATCH] genetlink: fix unsigned int comparison with less than zero
On Sat, Nov 12, 2016 at 1:37 PM, Johannes Berg wrote: > On Thu, 2016-11-10 at 09:11 -0800, Cong Wang wrote: >> On Thu, Nov 10, 2016 at 7:57 AM, Colin King > > wrote: >> > >> > From: Colin Ian King >> > >> > family->id is unsigned, so the less than zero check for >> > failure return from idr_alloc is never true and so the error exit >> > is never handled. Instead, assign err and check if this is less >> > than zero since this is a signed integer. >> >> Why family->id can't be just signed int? For me it should be. > > I suppose it could be, since family IDs are allocated in a 16-bit range > anyway. But family IDs can also never actually be negative, so having > an unsigned int in the struct makes sense too. All idr_* API's accept int, rather than unsigned int. This is my point.
Re: [PATCH] genetlink: fix unsigned int comparison with less than zero
On Thu, 2016-11-10 at 09:11 -0800, Cong Wang wrote: > On Thu, Nov 10, 2016 at 7:57 AM, Colin King > wrote: > > > > From: Colin Ian King > > > > family->id is unsigned, so the less than zero check for > > failure return from idr_alloc is never true and so the error exit > > is never handled. Instead, assign err and check if this is less > > than zero since this is a signed integer. > > Why family->id can't be just signed int? For me it should be. I suppose it could be, since family IDs are allocated in a 16-bit range anyway. But family IDs can also never actually be negative, so having an unsigned int in the struct makes sense too. I tend to think this patch is fine. johannes
Re: [PATCH] genetlink: fix unsigned int comparison with less than zero
On Thu, Nov 10, 2016 at 7:57 AM, Colin King wrote: > From: Colin Ian King > > family->id is unsigned, so the less than zero check for > failure return from idr_alloc is never true and so the error exit > is never handled. Instead, assign err and check if this is less > than zero since this is a signed integer. Why family->id can't be just signed int? For me it should be.
[PATCH] genetlink: fix unsigned int comparison with less than zero
From: Colin Ian King family->id is unsigned, so the less than zero check for failure return from idr_alloc is never true and so the error exit is never handled. Instead, assign err and check if this is less than zero since this is a signed integer. Issue found with static analysis with CoverityScan, CID 1375916 Signed-off-by: Colin Ian King --- net/netlink/genetlink.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index f0b65fe..2ea61ba 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -360,12 +360,10 @@ int genl_register_family(struct genl_family *family) } else family->attrbuf = NULL; - family->id = idr_alloc(&genl_fam_idr, family, + family->id = err = idr_alloc(&genl_fam_idr, family, start, end + 1, GFP_KERNEL); - if (family->id < 0) { - err = family->id; + if (err < 0) goto errout_free; - } err = genl_validate_assign_mc_groups(family); if (err) -- 2.10.2