netdev - Re: [PATCH] Fix Warnings from net/netlink/genetlink.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 11 Aug 2009 21:03:55 -0700
From:	Marcel Holtmann <marcel@...tmann.org>
To:	Stephen Rothwell <sfr@...b.auug.org.au>
Cc:	Stephen Hemminger <shemminger@...tta.com>,
	vibi_sreenivasan@....com, netdev <netdev@...r.kernel.org>,
	Johannes Berg <johannes@...solutions.net>,
	Thomas Graf <tgraf@...g.ch>,
	linux-next <linux-next@...r.kernel.org>
Subject: Re: [PATCH] Fix Warnings from net/netlink/genetlink.c

Hi Stephen,

> > > > @@ -136,7 +136,7 @@ int genl_register_mc_group(struct genl_family *family,
> > > >  {
> > > >         int id;
> > > >         unsigned long *new_groups;
> > > > -       int err;
> > > > +       int err = 0;
> > > >  
> > > >         BUG_ON(grp->name[0] == '\0');
> > > 
> > > this looks fishy. How does gcc thinks this variable is uninitialized. If
> > > I look at the code in Linus' tree, I don't see it.
> > 
> > Agreed, and the line numbers are off.
> 
> In the -next tree, it looks like this:

it would have been nice if the patch actually indicates that it is for
-next since otherwise just shutting up a compiler warning is a bad idea
in this case.

> int genl_register_mc_group(struct genl_family *family,
> 			   struct genl_multicast_group *grp)
> {
> 	int id;
> 	unsigned long *new_groups;
> 	int err;
> 
> 	BUG_ON(grp->name[0] == '\0');
> 
> 	genl_lock();
> 
> 	/* special-case our own group */
> 	if (grp == &notify_grp)
> 		id = GENL_ID_CTRL;
> 	else
> 		id = find_first_zero_bit(mc_groups,
> 					 mc_groups_longs * BITS_PER_LONG);
> 
> 
> 	if (id >= mc_groups_longs * BITS_PER_LONG) {
> 		size_t nlen = (mc_groups_longs + 1) * sizeof(unsigned long);
> 
> 		if (mc_groups == &mc_group_start) {
> 			new_groups = kzalloc(nlen, GFP_KERNEL);
> 			if (!new_groups) {
> 				err = -ENOMEM;
> 				goto out;
> 			}
> 			mc_groups = new_groups;
> 			*mc_groups = mc_group_start;
> 		} else {
> 			new_groups = krealloc(mc_groups, nlen, GFP_KERNEL);
> 			if (!new_groups) {
> 				err = -ENOMEM;
> 				goto out;
> 			}
> 			mc_groups = new_groups;
> 			mc_groups[mc_groups_longs] = 0;
> 		}
> 		mc_groups_longs++;
> 	}
> 
> 	if (family->netnsok) {
> 		struct net *net;
> 
> 		rcu_read_lock();
> 		for_each_net_rcu(net) {
> 			err = netlink_change_ngroups(net->genl_sock,
> 					mc_groups_longs * BITS_PER_LONG);
> 			if (err) {
> 				rcu_read_unlock();
> 				goto out;
> 			}
> 		}
> 		rcu_read_unlock();
> 	} else {
> 		err = netlink_change_ngroups(init_net.genl_sock,
> 					     mc_groups_longs * BITS_PER_LONG);
> 		if (err)
> 			goto out;
> 	}
> 
> 	grp->id = id;
> 	set_bit(id, mc_groups);
> 	list_add_tail(&grp->list, &family->mcast_groups);
> 	grp->family = family;
> 
> 	genl_ctrl_event(CTRL_CMD_NEWMCAST_GRP, grp);
>  out:
> 	genl_unlock();
> 	return err;
> }
> 
> so if family->netnsok is true and the for_each_net_rcu loop executes 0
> times, err will not be set ...  Now, that may not be logically possible,
> but I can't tell that from this code.

I prefer we add a err = 0 in the if (family->netnsok) { block instead of
just globally setting it to a value.

Regards

Marcel


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html