Hello Johannes Berg,
The patch 9dd1953846c7: "wifi: nl80211/mac80211: clarify link ID in
control port TX" from Jul 19, 2022, leads to the following Smatch
static checker warning:
net/mac80211/tx.c:5902 ieee80211_tx_control_port()
error: potential NULL/IS_ERR bug 'sta'
net/mac80211/tx.c
5807 int ieee80211_tx_control_port(struct wiphy *wiphy, struct net_device *dev,
5808 const u8 *buf, size_t len,
5809 const u8 *dest, __be16 proto, bool unencrypted,
5810 int link_id, u64 *cookie)
5811 {
5812 struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
5813 struct ieee80211_local *local = sdata->local;
5814 struct sta_info *sta;
5815 struct sk_buff *skb;
5816 struct ethhdr *ehdr;
5817 u32 ctrl_flags = 0;
5818 u32 flags = 0;
5819 int err;
5820
5821 /* Only accept CONTROL_PORT_PROTOCOL configured in CONNECT/ASSOCIATE
5822 * or Pre-Authentication
5823 */
5824 if (proto != sdata->control_port_protocol &&
5825 proto != cpu_to_be16(ETH_P_PREAUTH))
5826 return -EINVAL;
5827
5828 if (proto == sdata->control_port_protocol)
5829 ctrl_flags |= IEEE80211_TX_CTRL_PORT_CTRL_PROTO |
5830 IEEE80211_TX_CTRL_SKIP_MPATH_LOOKUP;
5831
5832 if (unencrypted)
5833 flags |= IEEE80211_TX_INTFL_DONT_ENCRYPT;
5834
5835 if (cookie)
5836 ctrl_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
5837
5838 flags |= IEEE80211_TX_INTFL_NL80211_FRAME_TX;
5839
5840 skb = dev_alloc_skb(local->hw.extra_tx_headroom +
5841 sizeof(struct ethhdr) + len);
5842 if (!skb)
5843 return -ENOMEM;
5844
5845 skb_reserve(skb, local->hw.extra_tx_headroom + sizeof(struct ethhdr));
5846
5847 skb_put_data(skb, buf, len);
5848
5849 ehdr = skb_push(skb, sizeof(struct ethhdr));
5850 memcpy(ehdr->h_dest, dest, ETH_ALEN);
5851
5852 /* we may override the SA for MLO STA later */
5853 if (link_id < 0) {
5854 ctrl_flags |= u32_encode_bits(IEEE80211_LINK_UNSPECIFIED,
5855 IEEE80211_TX_CTRL_MLO_LINK);
5856 memcpy(ehdr->h_source, sdata->vif.addr, ETH_ALEN);
5857 } else {
5858 struct ieee80211_bss_conf *link_conf;
5859
5860 ctrl_flags |= u32_encode_bits(link_id,
5861 IEEE80211_TX_CTRL_MLO_LINK);
5862
5863 rcu_read_lock();
5864 link_conf = rcu_dereference(sdata->vif.link_conf[link_id]);
5865 if (!link_conf) {
5866 dev_kfree_skb(skb);
5867 rcu_read_unlock();
5868 return -ENOLINK;
5869 }
5870 memcpy(ehdr->h_source, link_conf->addr, ETH_ALEN);
5871 rcu_read_unlock();
5872 }
5873
5874 ehdr->h_proto = proto;
5875
5876 skb->dev = dev;
5877 skb->protocol = proto;
5878 skb_reset_network_header(skb);
5879 skb_reset_mac_header(skb);
5880
5881 /* update QoS header to prioritize control port frames if possible,
5882 * priorization also happens for control port frames send over
5883 * AF_PACKET
5884 */
5885 rcu_read_lock();
5886 err = ieee80211_lookup_ra_sta(sdata, skb, &sta);
For sdata->vif.type == NL80211_IFTYPE_MESH_POINT then "sta" can be NULL.
Smatch doesn't know the value of sdata->vif.type at this point, and even
if it did, then it doesn't split the return states up with enough
granularity for that to make a difference.
5887 if (err) {
5888 rcu_read_unlock();
5889 return err;
5890 }
5891
5892 if (!IS_ERR(sta)) {
5893 u16 queue = __ieee80211_select_queue(sdata, sta, skb);
5894
5895 skb_set_queue_mapping(skb, queue);
5896 skb_get_hash(skb);
5897
5898 /*
5899 * for MLO STA, the SA should be the AP MLD address, but
5900 * the link ID has been selected already
5901 */
--> 5902 if (sta->sta.mlo)
Which Smatch complains about here.
5903 memcpy(ehdr->h_source, sdata->vif.addr, ETH_ALEN);
5904 }
5905 rcu_read_unlock();
5906
5907 /* mutex lock is only needed for incrementing the cookie counter */
5908 mutex_lock(&local->mtx);
5909
5910 local_bh_disable();
5911 __ieee80211_subif_start_xmit(skb, skb->dev, flags, ctrl_flags, cookie);
5912 local_bh_enable();
5913
5914 mutex_unlock(&local->mtx);
5915
5916 return 0;
5917 }
regards,
dan carpenter
Hi Dan,
On Tue, 2022-07-26 at 17:10 +0300, Dan Carpenter wrote:
> 5885 rcu_read_lock();
> 5886 err = ieee80211_lookup_ra_sta(sdata, skb, &sta);
>
> For sdata->vif.type == NL80211_IFTYPE_MESH_POINT then "sta" can be NULL.
> Smatch doesn't know the value of sdata->vif.type at this point, and even
> if it did, then it doesn't split the return states up with enough
> granularity for that to make a difference.
Right, but that wouldn't matter anyway?
> 5887 if (err) {
> 5888 rcu_read_unlock();
> 5889 return err;
> 5890 }
> 5891
> 5892 if (!IS_ERR(sta)) {
Since this is all independent of the interface type.
> 5893 u16 queue = __ieee80211_select_queue(sdata, sta, skb);
> 5894
> 5895 skb_set_queue_mapping(skb, queue);
> 5896 skb_get_hash(skb);
> 5897
> 5898 /*
> 5899 * for MLO STA, the SA should be the AP MLD address, but
> 5900 * the link ID has been selected already
> 5901 */
> --> 5902 if (sta->sta.mlo)
>
> Which Smatch complains about here.
Makes sense, should be "if (sta && sta->sta.mlo)" then I suppose. I'm on
vacation now, so if you want to send a patch as a reminder that'd be
nice.
I might clean this up later - it's not the first time the strange return
behaviour semantics of ieee80211_lookup_ra_sta() have gotten confusing
...
Thanks!
johannes
On Tue, Jul 26, 2022 at 04:42:41PM +0200, Johannes Berg wrote:
> Hi Dan,
>
>
> On Tue, 2022-07-26 at 17:10 +0300, Dan Carpenter wrote:
> > 5885 rcu_read_lock();
> > 5886 err = ieee80211_lookup_ra_sta(sdata, skb, &sta);
> >
> > For sdata->vif.type == NL80211_IFTYPE_MESH_POINT then "sta" can be NULL.
> > Smatch doesn't know the value of sdata->vif.type at this point, and even
> > if it did, then it doesn't split the return states up with enough
> > granularity for that to make a difference.
>
> Right, but that wouldn't matter anyway?
>
> > 5887 if (err) {
> > 5888 rcu_read_unlock();
> > 5889 return err;
> > 5890 }
> > 5891
> > 5892 if (!IS_ERR(sta)) {
>
> Since this is all independent of the interface type.
>
> > 5893 u16 queue = __ieee80211_select_queue(sdata, sta, skb);
> > 5894
> > 5895 skb_set_queue_mapping(skb, queue);
> > 5896 skb_get_hash(skb);
> > 5897
> > 5898 /*
> > 5899 * for MLO STA, the SA should be the AP MLD address, but
> > 5900 * the link ID has been selected already
> > 5901 */
> > --> 5902 if (sta->sta.mlo)
> >
> > Which Smatch complains about here.
>
> Makes sense, should be "if (sta && sta->sta.mlo)" then I suppose. I'm on
> vacation now, so if you want to send a patch as a reminder that'd be
> nice.
Sure. I can do that. Enjoy your vacation!
regards,
dan carpenter