Re: [PATCH v2 03/10] nl80211: Prefer ether_addr_copy

[Johannes Berg]

> 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

Re: [PATCH v2 03/10] nl80211: Prefer ether_addr_copy

[Kirtika Ruchandani]

> 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.

Re: [PATCH v2 03/10] nl80211: Prefer ether_addr_copy

[Julian Calaby]

Hi All,

On Mon, May 30, 2016 at 12:52 PM, Kirtika Ruchandani
 wrote:
> This patch fixes the checkpatch,pl to prefer ether_addr_copy
> over memcpy.
>
> Signed-off-by: Kirtika Ruchandani 

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
>



-- 
Julian Calaby

Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/