Patchwork Fix Warnings from net/netlink/genetlink.c

login
register
mail settings
Submitter vibi sreenivasan
Date Aug. 11, 2009, 10:07 a.m.
Message ID <1249985224.13065.11.camel@HunTEr>
Download mbox | patch
Permalink /patch/31144/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

vibi sreenivasan - Aug. 11, 2009, 10:07 a.m.
net/netlink/genetlink.c: In function `genl_register_mc_group':
net/netlink/genetlink.c:139: warning: 'err' might be used uninitialized in this function

Signed-off-by: vibi sreenivasan <vibi_sreenivasan@cms.com>
---
 net/netlink/genetlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Marcel Holtmann - Aug. 11, 2009, 11:57 p.m.
Hi Vibi,

> net/netlink/genetlink.c: In function `genl_register_mc_group':
> net/netlink/genetlink.c:139: warning: 'err' might be used uninitialized in this function
> 
> Signed-off-by: vibi sreenivasan <vibi_sreenivasan@cms.com>
> ---
>  net/netlink/genetlink.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 575c643..66f6ba0 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -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.

Regards

Marcel


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - Aug. 12, 2009, 3:24 a.m.
On Tue, 11 Aug 2009 16:57:41 -0700
Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Vibi,
> 
> > net/netlink/genetlink.c: In function `genl_register_mc_group':
> > net/netlink/genetlink.c:139: warning: 'err' might be used uninitialized in this function
> > 
> > Signed-off-by: vibi sreenivasan <vibi_sreenivasan@cms.com>
> > ---
> >  net/netlink/genetlink.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> > index 575c643..66f6ba0 100644
> > --- a/net/netlink/genetlink.c
> > +++ b/net/netlink/genetlink.c
> > @@ -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.
Stephen Rothwell - Aug. 12, 2009, 3:50 a.m.
Hi all,

On Tue, 11 Aug 2009 20:24:59 -0700 Stephen Hemminger <shemminger@vyatta.com> wrote:
>
> > > @@ -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:

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.
Marcel Holtmann - Aug. 12, 2009, 4:03 a.m.
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 575c643..66f6ba0 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -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');