2023-12-13 17:07:35

by Jeff Johnson

[permalink] [raw]
Subject: [PATCH 6/6] wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update

In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update()
the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function
ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it.

No functional changes, compile tested only.

[1] https://lore.kernel.org/linux-wireless/[email protected]/

Signed-off-by: Jeff Johnson <[email protected]>
---
drivers/net/wireless/ath/ath10k/wmi.c | 2 --
1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 4d5aadbc7159..0cfd9484c45e 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -8918,8 +8918,6 @@ ath10k_wmi_10_4_gen_tdls_peer_update(struct ath10k *ar,
if (!skb)
return ERR_PTR(-ENOMEM);

- memset(skb->data, 0, sizeof(*cmd));
-
cmd = (struct wmi_10_4_tdls_peer_update_cmd *)skb->data;
cmd->vdev_id = __cpu_to_le32(arg->vdev_id);
ether_addr_copy(cmd->peer_macaddr.addr, arg->addr);

--
2.42.0



2023-12-13 19:16:07

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH 6/6] wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update

On Wed, Dec 13, 2023 at 09:06:44AM -0800, Jeff Johnson wrote:
> In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update()
> the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function
> ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it.

Is .gen_tdls_peer_update only ever called after a fresh allocation? It
wasn't obvious to me as I tried to follow the call paths. Is there harm
in leaving this?

-Kees

>
> No functional changes, compile tested only.
>
> [1] https://lore.kernel.org/linux-wireless/[email protected]/
>
> Signed-off-by: Jeff Johnson <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/wmi.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index 4d5aadbc7159..0cfd9484c45e 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -8918,8 +8918,6 @@ ath10k_wmi_10_4_gen_tdls_peer_update(struct ath10k *ar,
> if (!skb)
> return ERR_PTR(-ENOMEM);
>
> - memset(skb->data, 0, sizeof(*cmd));
> -
> cmd = (struct wmi_10_4_tdls_peer_update_cmd *)skb->data;
> cmd->vdev_id = __cpu_to_le32(arg->vdev_id);
> ether_addr_copy(cmd->peer_macaddr.addr, arg->addr);
>
> --
> 2.42.0
>

--
Kees Cook

2023-12-13 19:36:36

by Jeff Johnson

[permalink] [raw]
Subject: Re: [PATCH 6/6] wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update

On 12/13/2023 11:16 AM, Kees Cook wrote:
> On Wed, Dec 13, 2023 at 09:06:44AM -0800, Jeff Johnson wrote:
>> In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update()
>> the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function
>> ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it.
>
> Is .gen_tdls_peer_update only ever called after a fresh allocation? It
> wasn't obvious to me as I tried to follow the call paths. Is there harm
> in leaving this?

The only harm is a slight increase in code size and cpu cycles.

However note the skb allocation is done within
ath10k_wmi_10_4_gen_tdls_peer_update() itself, just before the code
being removed:
skb = ath10k_wmi_alloc_skb(ar, len);
if (!skb)
return ERR_PTR(-ENOMEM);

And in ath10k_wmi_alloc_skb() we have:
memset(skb->data, 0, round_len);

So the memset() being removed is always redundant.

/jeff

2023-12-13 19:37:18

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH 6/6] wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update

On Wed, Dec 13, 2023 at 11:36:08AM -0800, Jeff Johnson wrote:
> On 12/13/2023 11:16 AM, Kees Cook wrote:
> > On Wed, Dec 13, 2023 at 09:06:44AM -0800, Jeff Johnson wrote:
> >> In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update()
> >> the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function
> >> ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it.
> >
> > Is .gen_tdls_peer_update only ever called after a fresh allocation? It
> > wasn't obvious to me as I tried to follow the call paths. Is there harm
> > in leaving this?
>
> The only harm is a slight increase in code size and cpu cycles.
>
> However note the skb allocation is done within
> ath10k_wmi_10_4_gen_tdls_peer_update() itself, just before the code
> being removed:
> skb = ath10k_wmi_alloc_skb(ar, len);
> if (!skb)
> return ERR_PTR(-ENOMEM);
>
> And in ath10k_wmi_alloc_skb() we have:
> memset(skb->data, 0, round_len);
>
> So the memset() being removed is always redundant.

LOL. I see now. I missed that was was looking outside the function! :P

Reviewed-by: Kees Cook <[email protected]>

--
Kees Cook

2023-12-13 20:20:17

by Gustavo A. R. Silva

[permalink] [raw]
Subject: Re: [PATCH 6/6] wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update



On 12/13/23 11:06, Jeff Johnson wrote:
> In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update()
> the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function
> ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it.
>
> No functional changes, compile tested only.
>
> [1] https://lore.kernel.org/linux-wireless/[email protected]/
>
> Signed-off-by: Jeff Johnson <[email protected]>

Reviewed-by: Gustavo A. R. Silva <[email protected]>

Thanks!
--
Gustavo

> ---
> drivers/net/wireless/ath/ath10k/wmi.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index 4d5aadbc7159..0cfd9484c45e 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -8918,8 +8918,6 @@ ath10k_wmi_10_4_gen_tdls_peer_update(struct ath10k *ar,
> if (!skb)
> return ERR_PTR(-ENOMEM);
>
> - memset(skb->data, 0, sizeof(*cmd));
> -
> cmd = (struct wmi_10_4_tdls_peer_update_cmd *)skb->data;
> cmd->vdev_id = __cpu_to_le32(arg->vdev_id);
> ether_addr_copy(cmd->peer_macaddr.addr, arg->addr);
>