re: mac80211: support P2P Device abstraction

re: mac80211: support P2P Device abstraction

[Dan Carpenter]

Hello Johannes Berg,

This is a semi-automatic email about new static checker warnings.

The patch f142c6b906da: "mac80211: support P2P Device abstraction" 
from Jun 18, 2012, leads to the following Smatch complaint:

net/mac80211/iface.c:1168 ieee80211_setup_sdata()
	 error: we previously assumed 'sdata->dev' could be null (see line 1134)

net/mac80211/iface.c
  1133		/* only monitor/p2p-device differ */
  1134		if (sdata->dev) {
                    ^^^^^^^^^^
New test.

  1135			sdata->dev->netdev_ops = &ieee80211_dataif_ops;
  1136			sdata->dev->type = ARPHRD_ETHER;
  1137		}

[snip]

  1167		case NL80211_IFTYPE_MONITOR:
  1168			sdata->dev->type = ARPHRD_IEEE80211_RADIOTAP;
                        ^^^^^^^^^^^^
Old dereference.

  1169			sdata->dev->netdev_ops = &ieee80211_monitorif_ops;
  1170			sdata->u.mntr_flags = MONITOR_FLAG_CONTROL |

regards,
dan carpenter

Re: mac80211: support P2P Device abstraction

[Johannes Berg]

Hi Dan,

> The patch f142c6b906da: "mac80211: support P2P Device abstraction" 
> from Jun 18, 2012, leads to the following Smatch complaint:
> 
> net/mac80211/iface.c:1168 ieee80211_setup_sdata()
> 	 error: we previously assumed 'sdata->dev' could be null (see line 1134)
> 
> net/mac80211/iface.c
>   1133		/* only monitor/p2p-device differ */
>   1134		if (sdata->dev) {
>                     ^^^^^^^^^^
> New test.
> 
>   1135			sdata->dev->netdev_ops = &ieee80211_dataif_ops;
>   1136			sdata->dev->type = ARPHRD_ETHER;
>   1137		}
> 
> [snip]
> 
>   1167		case NL80211_IFTYPE_MONITOR:
>   1168			sdata->dev->type = ARPHRD_IEEE80211_RADIOTAP;
>                         ^^^^^^^^^^^^
> Old dereference.

Thanks. I'm aware of the warning (I run smatch), but sdata->dev can only
be NULL in "case NL80211_IFTYPE_P2P_DEVICE", so adding an extra check in
the monitor case didn't seem worthwhile.

It might have been better to use sdata->vif.type != P2P_DEVICE in the
new test I guess, but then it wouldn't be extensible to new interface
types like Bluetooth AMP.

johannes

Re: mac80211: support P2P Device abstraction

[Dan Carpenter]

On Tue, Sep 04, 2012 at 06:18:45PM +0200, Johannes Berg wrote:
> Hi Dan,
> 
> > The patch f142c6b906da: "mac80211: support P2P Device abstraction" 
> > from Jun 18, 2012, leads to the following Smatch complaint:
> > 
> > net/mac80211/iface.c:1168 ieee80211_setup_sdata()
> > 	 error: we previously assumed 'sdata->dev' could be null (see line 1134)
> > 
> > net/mac80211/iface.c
> >   1133		/* only monitor/p2p-device differ */
> >   1134		if (sdata->dev) {
> >                     ^^^^^^^^^^
> > New test.
> > 
> >   1135			sdata->dev->netdev_ops = &ieee80211_dataif_ops;
> >   1136			sdata->dev->type = ARPHRD_ETHER;
> >   1137		}
> > 
> > [snip]
> > 
> >   1167		case NL80211_IFTYPE_MONITOR:
> >   1168			sdata->dev->type = ARPHRD_IEEE80211_RADIOTAP;
> >                         ^^^^^^^^^^^^
> > Old dereference.
> 
> Thanks. I'm aware of the warning (I run smatch), but sdata->dev can only
> be NULL in "case NL80211_IFTYPE_P2P_DEVICE", so adding an extra check in
> the monitor case didn't seem worthwhile.
> 
> It might have been better to use sdata->vif.type != P2P_DEVICE in the
> new test I guess, but then it wouldn't be extensible to new interface
> types like Bluetooth AMP.

Gar.  This was my bad.  I didn't realize the warning was so old.  I
try to only send the automatic emails for new warnings.  I have been
meaning to fix the bug in my script for a while now but this time
I will really do that.

regards,
dan carpenter

Re: mac80211: support P2P Device abstraction

[Johannes Berg]

Hi Dan,

> > > The patch f142c6b906da: "mac80211: support P2P Device abstraction" 
> > > from Jun 18, 2012, leads to the following Smatch complaint:
> > > 
> > > net/mac80211/iface.c:1168 ieee80211_setup_sdata()
> > > 	 error: we previously assumed 'sdata->dev' could be null (see line 1134)
> > > 
> > > net/mac80211/iface.c
> > >   1133		/* only monitor/p2p-device differ */
> > >   1134		if (sdata->dev) {
> > >                     ^^^^^^^^^^
> > > New test.
> > > 
> > >   1135			sdata->dev->netdev_ops = &ieee80211_dataif_ops;
> > >   1136			sdata->dev->type = ARPHRD_ETHER;
> > >   1137		}
> > > 
> > > [snip]
> > > 
> > >   1167		case NL80211_IFTYPE_MONITOR:
> > >   1168			sdata->dev->type = ARPHRD_IEEE80211_RADIOTAP;
> > >                         ^^^^^^^^^^^^
> > > Old dereference.
> > 
> > Thanks. I'm aware of the warning (I run smatch), but sdata->dev can only
> > be NULL in "case NL80211_IFTYPE_P2P_DEVICE", so adding an extra check in
> > the monitor case didn't seem worthwhile.
> > 
> > It might have been better to use sdata->vif.type != P2P_DEVICE in the
> > new test I guess, but then it wouldn't be extensible to new interface
> > types like Bluetooth AMP.
> 
> Gar.  This was my bad.  I didn't realize the warning was so old.  I
> try to only send the automatic emails for new warnings.  I have been
> meaning to fix the bug in my script for a while now but this time
> I will really do that.

No no, the warning was introduced in that patch on June 18th, so in that
sense the email was correct. Before that patch, the test didn't exist,
so the warning didn't show up, because the function *always* assumed
sdata->dev was set. Now there's one case where this isn't the case any
more, for vif.type == NL80211_IFTYPE_P2P_DEVICE, but the monitor case
can still assume sdata->dev is set...

It's really just a false positive since sdata->dev and sdata->vif.type
are correlated, but smatch can't know that.

johannes