Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d68c4b38-88e1-02ef-5779-08b93fd0d47b@kernel.org>
Date:   Fri, 2 Jun 2023 06:58:31 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
To:     Shiji Yang <yangshiji66@outlook.com>,
        linux-wireless@vger.kernel.org
Cc:     Kalle Valo <kvalo@kernel.org>,
        Nick Kossifidis <mickflemm@gmail.com>,
        Luis Chamberlain <mcgrof@kernel.org>,
        =?UTF-8?Q?Toke_H=c3=b8iland-J=c3=b8rg?= =?UTF-8?Q?ensen?= 
        <toke@toke.dk>, Robert Marko <robimarko@gmail.com>,
        Christian Lamparter <chunkeey@gmail.com>,
        Christian Marangi <ansuelsmth@gmail.com>,
        Shiji Yang <yangshiji66@qq.com>
References: <TYAP286MB0315EC437BF53C8DB2B5D022BC4EA@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM>
Content-Language: en-US
From:   Jiri Slaby <jirislaby@kernel.org>
Subject: Re: [PATCH] wifi: ath: add struct_group for struct ath_cycle_counters
In-Reply-To: <TYAP286MB0315EC437BF53C8DB2B5D022BC4EA@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 02. 06. 23, 6:36, Shiji Yang wrote:
> From: Shiji Yang <yangshiji66@qq.com>
> 
> Add a struct_group to around all members in struct ath_cycle_counters.
> It can help the compiler detect the intended bounds of the memcpy() and
> memset().
> 
> This patch fixes the following build warning:
> 
> In function 'fortify_memset_chk',
>      inlined from 'ath9k_ps_wakeup' at drivers/net/wireless/ath/ath9k/main.c:140:3:
> ./include/linux/fortify-string.h:314:25: error: call to '__write_overflow_field' declared with attribute warning:
> detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>    314 |                         __write_overflow_field(p_size_field, size);
>        |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 

Hi,

what compiler/version is this with?

> Signed-off-by: Shiji Yang <yangshiji66@qq.com>
> ---
> More discussion on: https://github.com/openwrt/openwrt/pull/12764

No "__write_overflow_field" there. Is this the right link?

> ---
>   drivers/net/wireless/ath/ath.h                | 10 ++++++----
>   drivers/net/wireless/ath/ath5k/ani.c          |  2 +-
>   drivers/net/wireless/ath/ath5k/base.c         |  4 ++--
>   drivers/net/wireless/ath/ath5k/mac80211-ops.c |  2 +-
>   drivers/net/wireless/ath/ath9k/link.c         |  2 +-
>   drivers/net/wireless/ath/ath9k/main.c         |  4 ++--
>   drivers/net/wireless/ath/hw.c                 |  2 +-
>   7 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
> index f02a308a9..4b42e401a 100644
> --- a/drivers/net/wireless/ath/ath.h
> +++ b/drivers/net/wireless/ath/ath.h
> @@ -43,10 +43,12 @@ struct ath_ani {
>   };
>   
>   struct ath_cycle_counters {
> -	u32 cycles;
> -	u32 rx_busy;
> -	u32 rx_frame;
> -	u32 tx_frame;
> +	struct_group(cnts,
> +		u32 cycles;
> +		u32 rx_busy;
> +		u32 rx_frame;
> +		u32 tx_frame;
> +	);

This is horrid.

>   };
>   
>   enum ath_device_state {
> diff --git a/drivers/net/wireless/ath/ath5k/ani.c b/drivers/net/wireless/ath/ath5k/ani.c
> index 850c608b4..fa95f0f0f 100644
> --- a/drivers/net/wireless/ath/ath5k/ani.c
> +++ b/drivers/net/wireless/ath/ath5k/ani.c
> @@ -379,7 +379,7 @@ ath5k_hw_ani_get_listen_time(struct ath5k_hw *ah, struct ath5k_ani_state *as)
>   	spin_lock_bh(&common->cc_lock);
>   
>   	ath_hw_cycle_counters_update(common);
> -	memcpy(&as->last_cc, &common->cc_ani, sizeof(as->last_cc));
> +	memcpy(&as->last_cc.cnts, &common->cc_ani.cnts, sizeof(as->last_cc.cnts));

So is this.

Care to elaborate why this is needed at all, provided we copy/zero a 
whole structure? And describe it in the commit log, not in random 
external sources.

thanks,
-- 
js
suse labs