| Message ID | 20161110155758.26996-1-colin.king@canonical.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Nov 10, 2016 at 7:57 AM, Colin King <colin.king@canonical.com> wrote: > From: Colin Ian King <colin.king@canonical.com> > > 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.
On Thu, 2016-11-10 at 09:11 -0800, Cong Wang wrote: > On Thu, Nov 10, 2016 at 7:57 AM, Colin King <colin.king@canonical.com > > wrote: > > > > From: Colin Ian King <colin.king@canonical.com> > > > > 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
On Sat, Nov 12, 2016 at 1:37 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2016-11-10 at 09:11 -0800, Cong Wang wrote: >> On Thu, Nov 10, 2016 at 7:57 AM, Colin King <colin.king@canonical.com >> > wrote: >> > >> > From: Colin Ian King <colin.king@canonical.com> >> > >> > 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.
> > 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
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)