| Message ID | 49c03996365360077fe7317d108072c30354b555.1464576022.git.kirtika.ruchandani@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
Hi All, On Mon, May 30, 2016 at 12:52 PM, Kirtika Ruchandani <kirtika.ruchandani@gmail.com> wrote: > This patch fixes the checkpatch,pl to prefer ether_addr_copy > over memcpy. > > Signed-off-by: Kirtika Ruchandani <kirtika.ruchandani@gmail.com> This looks right to me, but doesn't ether_addr_copy() have alignment requirements? Could someone more familiar with that review these changes to ensure they're met? Thanks, Julian Calaby > --- > net/wireless/nl80211.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c > index cd422bd..34b8fbe 100644 > --- a/net/wireless/nl80211.c > +++ b/net/wireless/nl80211.c > @@ -3194,7 +3194,7 @@ static struct cfg80211_acl_data *parse_acl_data(struct wiphy *wiphy, > return ERR_PTR(-ENOMEM); > > nla_for_each_nested(attr, info->attrs[NL80211_ATTR_MAC_ADDRS], tmp) { > - memcpy(acl->mac_addrs[i].addr, nla_data(attr), ETH_ALEN); > + ether_addr_copy(acl->mac_addrs[i].addr, nla_data(attr)); > i++; > } > > @@ -5892,8 +5892,8 @@ static int nl80211_parse_random_mac(struct nlattr **attrs, > if (!attrs[NL80211_ATTR_MAC] || !attrs[NL80211_ATTR_MAC_MASK]) > return -EINVAL; > > - memcpy(mac_addr, nla_data(attrs[NL80211_ATTR_MAC]), ETH_ALEN); > - memcpy(mac_addr_mask, nla_data(attrs[NL80211_ATTR_MAC_MASK]), ETH_ALEN); > + ether_addr_copy(mac_addr, nla_data(attrs[NL80211_ATTR_MAC])); > + ether_addr_copy(mac_addr_mask, nla_data(attrs[NL80211_ATTR_MAC_MASK])); > > /* don't allow or configure an mcast address */ > if (!is_multicast_ether_addr(mac_addr_mask) || > @@ -9405,8 +9405,7 @@ static int nl80211_parse_wowlan_tcp(struct cfg80211_registered_device *rdev, > return -ENOMEM; > cfg->src = nla_get_in_addr(tb[NL80211_WOWLAN_TCP_SRC_IPV4]); > cfg->dst = nla_get_in_addr(tb[NL80211_WOWLAN_TCP_DST_IPV4]); > - memcpy(cfg->dst_mac, nla_data(tb[NL80211_WOWLAN_TCP_DST_MAC]), > - ETH_ALEN); > + ether_addr_copy(cfg->dst_mac, nla_data(tb[NL80211_WOWLAN_TCP_DST_MAC])); > if (tb[NL80211_WOWLAN_TCP_SRC_PORT]) > port = nla_get_u16(tb[NL80211_WOWLAN_TCP_SRC_PORT]); > else > -- > 2.8.0.rc3.226.g39d4020 >
> This looks right to me, but doesn't ether_addr_copy() have alignment > requirements? Could someone more familiar with that review these > changes to ensure they're met? Thanks for catching this. The requirement is to be __aligned(2). I've added 4 instances of ether_addr_copy with 8 addresses as arguments. Of these, the 4 src arguments are really the same type (i.e. nla_data acting on a const nlattr*), so I'll try to reason about the 5 total cases below - 1. cfg->dst_mac should be 16-bit aligned due to the layout of struct cfg80211_wowlan_tcp. Its offset is 10 or 12 bytes in the structure depending on the system. 2 and 3. For mac_addr and mac_addr_mask, nl80211_parse_random_mac takes these in as u8* (and hence does not guarantee alignment?) Both the callers of this function today pass in arguments that are explicitly __aligned(2). But this cannot be said of future potential callers - so perhaps my patch introduces a bug? 4. Based on struct cfg80211_acl_data, acl->mac_addrs[i] should be not guaranteed to be __aligned(2). 5. For all the nla_data src arguments, the nla_data function returns ((char*) foo + 5) for pointer foo. So likely not __aligned(2). Based on 3, 4 and 5, this patch should be revoked, but it would be nice to have a confirmation from someone else. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> The requirement is to be __aligned(2). I've added 4 instances of > ether_addr_copy with 8 addresses as arguments. Of these, the 4 > src arguments are really the same type (i.e. nla_data acting on a > const nlattr*), so I'll try to reason about the 5 total cases below - > 1. cfg->dst_mac should be 16-bit aligned due to the layout of > struct cfg80211_wowlan_tcp. Its offset is 10 or 12 bytes in the > structure depending on the system. I wouldn't want to rely on that, since internal structures can be changed pretty easily. If necessary, add __aligned(2) to the struct members where appropriate. > 2 and 3. For mac_addr and mac_addr_mask, nl80211_parse_random_mac > takes these in as u8* (and hence does not guarantee alignment?) > Both the callers of this function today pass in arguments that are > explicitly __aligned(2). But this cannot be said of future potential > callers > - so perhaps my patch introduces a bug? That should be documented, but it's also pretty tricky to review/maintain that. I general, I don't really see much reason to use ether_addr_copy() in any of these code paths - it's not really a performance thing (in part due to the alignment, ether_addr_copy can be faster) > 4. Based on struct cfg80211_acl_data, acl->mac_addrs[i] should be not > guaranteed to be __aligned(2). See above. > 5. For all the nla_data src arguments, the nla_data function returns > ((char*) foo + 5) for pointer foo. So likely not __aligned(2). Those should be aligned since the data is copied into an SKB, and +5 doesn't seem right - nla_data() should be +4. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index cd422bd..34b8fbe 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -3194,7 +3194,7 @@ static struct cfg80211_acl_data *parse_acl_data(struct wiphy *wiphy, return ERR_PTR(-ENOMEM); nla_for_each_nested(attr, info->attrs[NL80211_ATTR_MAC_ADDRS], tmp) { - memcpy(acl->mac_addrs[i].addr, nla_data(attr), ETH_ALEN); + ether_addr_copy(acl->mac_addrs[i].addr, nla_data(attr)); i++; } @@ -5892,8 +5892,8 @@ static int nl80211_parse_random_mac(struct nlattr **attrs, if (!attrs[NL80211_ATTR_MAC] || !attrs[NL80211_ATTR_MAC_MASK]) return -EINVAL; - memcpy(mac_addr, nla_data(attrs[NL80211_ATTR_MAC]), ETH_ALEN); - memcpy(mac_addr_mask, nla_data(attrs[NL80211_ATTR_MAC_MASK]), ETH_ALEN); + ether_addr_copy(mac_addr, nla_data(attrs[NL80211_ATTR_MAC])); + ether_addr_copy(mac_addr_mask, nla_data(attrs[NL80211_ATTR_MAC_MASK])); /* don't allow or configure an mcast address */ if (!is_multicast_ether_addr(mac_addr_mask) || @@ -9405,8 +9405,7 @@ static int nl80211_parse_wowlan_tcp(struct cfg80211_registered_device *rdev, return -ENOMEM; cfg->src = nla_get_in_addr(tb[NL80211_WOWLAN_TCP_SRC_IPV4]); cfg->dst = nla_get_in_addr(tb[NL80211_WOWLAN_TCP_DST_IPV4]); - memcpy(cfg->dst_mac, nla_data(tb[NL80211_WOWLAN_TCP_DST_MAC]), - ETH_ALEN); + ether_addr_copy(cfg->dst_mac, nla_data(tb[NL80211_WOWLAN_TCP_DST_MAC])); if (tb[NL80211_WOWLAN_TCP_SRC_PORT]) port = nla_get_u16(tb[NL80211_WOWLAN_TCP_SRC_PORT]); else
This patch fixes the checkpatch,pl to prefer ether_addr_copy over memcpy. Signed-off-by: Kirtika Ruchandani <kirtika.ruchandani@gmail.com> --- net/wireless/nl80211.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) -- 2.8.0.rc3.226.g39d4020 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html