| Message ID | e595fd44-cf8a-ce14-8cc8-e3ecd4e8922a@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: Initialize return value in gro_cells_receive | expand |
On 10/6/20 8:53 PM, Gregory Rose wrote: > The 'res' return value is uninitalized and may be returned with > some random value. Initialize to NET_RX_DROP as the default > return value. > > Signed-off-by: Greg Rose <gvrose8192@gmail.com> > > diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c > index e095fb871d91..4e835960db07 100644 > --- a/net/core/gro_cells.c > +++ b/net/core/gro_cells.c > @@ -13,7 +13,7 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) > { > struct net_device *dev = skb->dev; > struct gro_cell *cell; > - int res; > + int res = NET_RX_DROP; > > rcu_read_lock(); > if (unlikely(!(dev->flags & IFF_UP))) I do not think this is needed. Also, when/if sending a patch fixing a bug, we require a Fixes: tag. Thanks.
On 10/7/2020 1:21 AM, Eric Dumazet wrote: > > > On 10/6/20 8:53 PM, Gregory Rose wrote: >> The 'res' return value is uninitalized and may be returned with >> some random value. Initialize to NET_RX_DROP as the default >> return value. >> >> Signed-off-by: Greg Rose <gvrose8192@gmail.com> >> >> diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c >> index e095fb871d91..4e835960db07 100644 >> --- a/net/core/gro_cells.c >> +++ b/net/core/gro_cells.c >> @@ -13,7 +13,7 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) >> { >> struct net_device *dev = skb->dev; >> struct gro_cell *cell; >> - int res; >> + int res = NET_RX_DROP; >> >> rcu_read_lock(); >> if (unlikely(!(dev->flags & IFF_UP))) > > I do not think this is needed. > > Also, when/if sending a patch fixing a bug, we require a Fixes: tag. > > Thanks. > If it's not needed then feel free to ignore it. It just looked like the unlikely case returns without setting the return value. Thanks - Greg Thanks,
On 10/7/20 5:50 PM, Gregory Rose wrote: > > > On 10/7/2020 1:21 AM, Eric Dumazet wrote: >> >> >> On 10/6/20 8:53 PM, Gregory Rose wrote: >>> The 'res' return value is uninitalized and may be returned with >>> some random value. Initialize to NET_RX_DROP as the default >>> return value. >>> >>> Signed-off-by: Greg Rose <gvrose8192@gmail.com> >>> >>> diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c >>> index e095fb871d91..4e835960db07 100644 >>> --- a/net/core/gro_cells.c >>> +++ b/net/core/gro_cells.c >>> @@ -13,7 +13,7 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) >>> { >>> struct net_device *dev = skb->dev; >>> struct gro_cell *cell; >>> - int res; >>> + int res = NET_RX_DROP; >>> >>> rcu_read_lock(); >>> if (unlikely(!(dev->flags & IFF_UP))) >> >> I do not think this is needed. >> >> Also, when/if sending a patch fixing a bug, we require a Fixes: tag. >> >> Thanks. >> > If it's not needed then feel free to ignore it. It just looked like > the unlikely case returns without setting the return value. Can you elaborate ? I do not see this problem in current upstream code. If a compiler gave you a warning, please give its version, thanks.
On 10/7/2020 9:37 AM, Eric Dumazet wrote: > > > On 10/7/20 5:50 PM, Gregory Rose wrote: >> >> >> On 10/7/2020 1:21 AM, Eric Dumazet wrote: >>> >>> >>> On 10/6/20 8:53 PM, Gregory Rose wrote: >>>> The 'res' return value is uninitalized and may be returned with >>>> some random value. Initialize to NET_RX_DROP as the default >>>> return value. >>>> >>>> Signed-off-by: Greg Rose <gvrose8192@gmail.com> >>>> >>>> diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c >>>> index e095fb871d91..4e835960db07 100644 >>>> --- a/net/core/gro_cells.c >>>> +++ b/net/core/gro_cells.c >>>> @@ -13,7 +13,7 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) >>>> { >>>> struct net_device *dev = skb->dev; >>>> struct gro_cell *cell; >>>> - int res; >>>> + int res = NET_RX_DROP; >>>> >>>> rcu_read_lock(); >>>> if (unlikely(!(dev->flags & IFF_UP))) >>> >>> I do not think this is needed. >>> >>> Also, when/if sending a patch fixing a bug, we require a Fixes: tag. >>> >>> Thanks. >>> >> If it's not needed then feel free to ignore it. It just looked like >> the unlikely case returns without setting the return value. > > Can you elaborate ? I do not see this problem in current upstream code. > > If a compiler gave you a warning, please give its version, thanks. > No, it's my misreading of the code - it jumps to the drop that is in the middle of an if statement, sets res to NET_RX_DROP there and then jumps to the unlock label. My apologies. - Greg
diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c index e095fb871d91..4e835960db07 100644 --- a/net/core/gro_cells.c +++ b/net/core/gro_cells.c @@ -13,7 +13,7 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) { struct net_device *dev = skb->dev; struct gro_cell *cell; - int res; + int res = NET_RX_DROP; rcu_read_lock();
The 'res' return value is uninitalized and may be returned with some random value. Initialize to NET_RX_DROP as the default return value. Signed-off-by: Greg Rose <gvrose8192@gmail.com> if (unlikely(!(dev->flags & IFF_UP)))