| Message ID | 20201026213040.3889546-8-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b96fab4e36023ee37ed1aad331ddd11b77c46c42 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | None | expand |
Arnd Bergmann <arnd@kernel.org> writes: > From: Arnd Bergmann <arnd@arndb.de> > > gcc-10 shows a false-positive warning with CONFIG_KASAN: > > drivers/net/wireless/ath/ath9k/dynack.c: In function 'ath_dynack_sample_tx_ts': > include/linux/etherdevice.h:290:14: warning: writing 4 bytes into a region of size 0 [-Wstringop-overflow=] > 290 | *(u32 *)dst = *(const u32 *)src; > | ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~ > > Until gcc is fixed, work around this by using memcpy() in place > of ether_addr_copy(). Hopefully gcc-11 will not have this problem. > > Link: https://godbolt.org/z/sab1MK > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97490 > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/net/wireless/ath/ath9k/dynack.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath9k/dynack.c b/drivers/net/wireless/ath/ath9k/dynack.c > index fbeb4a739d32..e4eb96b26ca4 100644 > --- a/drivers/net/wireless/ath/ath9k/dynack.c > +++ b/drivers/net/wireless/ath/ath9k/dynack.c > @@ -247,8 +247,14 @@ void ath_dynack_sample_tx_ts(struct ath_hw *ah, struct sk_buff *skb, > ridx = ts->ts_rateindex; > > da->st_rbf.ts[da->st_rbf.t_rb].tstamp = ts->ts_tstamp; > +#if defined(CONFIG_KASAN) && (CONFIG_GCC_VERSION >= 100000) && (CONFIG_GCC_VERSION < 110000) > + /* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97490 */ > + memcpy(da->st_rbf.addr[da->st_rbf.t_rb].h_dest, hdr->addr1, ETH_ALEN); > + memcpy(da->st_rbf.addr[da->st_rbf.t_rb].h_src, hdr->addr2, ETH_ALEN); > +#else > ether_addr_copy(da->st_rbf.addr[da->st_rbf.t_rb].h_dest, hdr->addr1); > ether_addr_copy(da->st_rbf.addr[da->st_rbf.t_rb].h_src, hdr->addr2); > +#endif Isn't there a better way to handle this? I really would not want checking for GCC versions become a common approach in drivers. I even think that using memcpy() always is better than the ugly ifdef.
On Mon, 2020-11-02 at 18:26 +0200, Kalle Valo wrote: > Arnd Bergmann <arnd@kernel.org> writes: > > > From: Arnd Bergmann <arnd@arndb.de> > > > > gcc-10 shows a false-positive warning with CONFIG_KASAN: > > > > drivers/net/wireless/ath/ath9k/dynack.c: In function 'ath_dynack_sample_tx_ts': > > include/linux/etherdevice.h:290:14: warning: writing 4 bytes into a region of size 0 [-Wstringop-overflow=] > > 290 | *(u32 *)dst = *(const u32 *)src; > > | ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~ > > > > Until gcc is fixed, work around this by using memcpy() in place > > of ether_addr_copy(). Hopefully gcc-11 will not have this problem. > > > > Link: https://godbolt.org/z/sab1MK > > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97490 > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > drivers/net/wireless/ath/ath9k/dynack.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/net/wireless/ath/ath9k/dynack.c b/drivers/net/wireless/ath/ath9k/dynack.c > > index fbeb4a739d32..e4eb96b26ca4 100644 > > --- a/drivers/net/wireless/ath/ath9k/dynack.c > > +++ b/drivers/net/wireless/ath/ath9k/dynack.c > > @@ -247,8 +247,14 @@ void ath_dynack_sample_tx_ts(struct ath_hw *ah, struct sk_buff *skb, > > ridx = ts->ts_rateindex; > > > > da->st_rbf.ts[da->st_rbf.t_rb].tstamp = ts->ts_tstamp; > > +#if defined(CONFIG_KASAN) && (CONFIG_GCC_VERSION >= 100000) && (CONFIG_GCC_VERSION < 110000) > > + /* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97490 */ > > + memcpy(da->st_rbf.addr[da->st_rbf.t_rb].h_dest, hdr->addr1, ETH_ALEN); > > + memcpy(da->st_rbf.addr[da->st_rbf.t_rb].h_src, hdr->addr2, ETH_ALEN); > > +#else > > ether_addr_copy(da->st_rbf.addr[da->st_rbf.t_rb].h_dest, hdr->addr1); > > ether_addr_copy(da->st_rbf.addr[da->st_rbf.t_rb].h_src, hdr->addr2); > > +#endif > > Isn't there a better way to handle this? I really would not want > checking for GCC versions become a common approach in drivers. > > I even think that using memcpy() always is better than the ugly ifdef. If you put memcpy() always somebody will surely go and clean it up to use ether_addr_copy() soon ... That said, if there's a gcc issue with ether_addr_copy() then how come it's specific to this place? johannes
On Mon, Nov 2, 2020 at 7:01 PM Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2020-11-02 at 18:26 +0200, Kalle Valo wrote: > > Arnd Bergmann <arnd@kernel.org> writes: > > > From: Arnd Bergmann <arnd@arndb.de> > > > > Isn't there a better way to handle this? I really would not want > > checking for GCC versions become a common approach in drivers. > > > > I even think that using memcpy() always is better than the ugly ifdef. > > If you put memcpy() always somebody will surely go and clean it up to > use ether_addr_copy() soon ... > > That said, if there's a gcc issue with ether_addr_copy() then how come > it's specific to this place? I have not been able to figure this out, hopefully some gcc developer eventually looks at the bug in more detail. Presumably it has something to do with the specific way the five levels of structures are nested here, and how things get inlined in this driver. If the bug happened everywhere, it would likely have been found and fixed earlier. Arnd
Johannes Berg <johannes@sipsolutions.net> writes: > On Mon, 2020-11-02 at 18:26 +0200, Kalle Valo wrote: >> Arnd Bergmann <arnd@kernel.org> writes: >> >> > From: Arnd Bergmann <arnd@arndb.de> >> > >> > gcc-10 shows a false-positive warning with CONFIG_KASAN: >> > >> > drivers/net/wireless/ath/ath9k/dynack.c: In function 'ath_dynack_sample_tx_ts': >> > include/linux/etherdevice.h:290:14: warning: writing 4 bytes into a region of size 0 [-Wstringop-overflow=] >> > 290 | *(u32 *)dst = *(const u32 *)src; >> > | ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~ >> > >> > Until gcc is fixed, work around this by using memcpy() in place >> > of ether_addr_copy(). Hopefully gcc-11 will not have this problem. >> > >> > Link: https://godbolt.org/z/sab1MK >> > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97490 >> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> > --- >> > drivers/net/wireless/ath/ath9k/dynack.c | 6 ++++++ >> > 1 file changed, 6 insertions(+) >> > >> > diff --git a/drivers/net/wireless/ath/ath9k/dynack.c b/drivers/net/wireless/ath/ath9k/dynack.c >> > index fbeb4a739d32..e4eb96b26ca4 100644 >> > --- a/drivers/net/wireless/ath/ath9k/dynack.c >> > +++ b/drivers/net/wireless/ath/ath9k/dynack.c >> > @@ -247,8 +247,14 @@ void ath_dynack_sample_tx_ts(struct ath_hw *ah, struct sk_buff *skb, >> > ridx = ts->ts_rateindex; >> > >> > da->st_rbf.ts[da->st_rbf.t_rb].tstamp = ts->ts_tstamp; >> > +#if defined(CONFIG_KASAN) && (CONFIG_GCC_VERSION >= 100000) && (CONFIG_GCC_VERSION < 110000) >> > + /* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97490 */ >> > + memcpy(da->st_rbf.addr[da->st_rbf.t_rb].h_dest, hdr->addr1, ETH_ALEN); >> > + memcpy(da->st_rbf.addr[da->st_rbf.t_rb].h_src, hdr->addr2, ETH_ALEN); >> > +#else >> > ether_addr_copy(da->st_rbf.addr[da->st_rbf.t_rb].h_dest, hdr->addr1); >> > ether_addr_copy(da->st_rbf.addr[da->st_rbf.t_rb].h_src, hdr->addr2); >> > +#endif >> >> Isn't there a better way to handle this? I really would not want >> checking for GCC versions become a common approach in drivers. >> >> I even think that using memcpy() always is better than the ugly ifdef. > > If you put memcpy() always somebody will surely go and clean it up to > use ether_addr_copy() soon ... I can always add a comment and hope that the cleanup people read comments :) I did that now in the pending branch: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=25cfc077bd7a798d1aa527ad2aa9932bb3284376 Does that look ok? I prefer that over the ifdef.
On Sat, Nov 7, 2020 at 12:21 PM Kalle Valo <kvalo@codeaurora.org> wrote: > Johannes Berg <johannes@sipsolutions.net> writes: > > On Mon, 2020-11-02 at 18:26 +0200, Kalle Valo wrote: > >> Arnd Bergmann <arnd@kernel.org> writes: > >> Isn't there a better way to handle this? I really would not want > >> checking for GCC versions become a common approach in drivers. > >> > >> I even think that using memcpy() always is better than the ugly ifdef. > > > > If you put memcpy() always somebody will surely go and clean it up to > > use ether_addr_copy() soon ... > > I can always add a comment and hope that the cleanup people read > comments :) I did that now in the pending branch: > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=25cfc077bd7a798d1aa527ad2aa9932bb3284376 > > Does that look ok? I prefer that over the ifdef. Fine with me. My original reason for the compiler version check was that we can eventually restore the previous version once the compiler is fixed for long enough that all broken compilers are too old to build the kernel, in maybe six years from now at the current rate of deprecating old compilers. Arnd
Arnd Bergmann <arnd@kernel.org> wrote: > gcc-10 shows a false-positive warning with CONFIG_KASAN: > > drivers/net/wireless/ath/ath9k/dynack.c: In function 'ath_dynack_sample_tx_ts': > include/linux/etherdevice.h:290:14: warning: writing 4 bytes into a region of size 0 [-Wstringop-overflow=] > 290 | *(u32 *)dst = *(const u32 *)src; > | ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~ > > Until gcc is fixed, work around this by using memcpy() in place > of ether_addr_copy(). Hopefully gcc-11 will not have this problem. > > Link: https://godbolt.org/z/sab1MK > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97490 > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > [kvalo@codeaurora.org: remove ifdef and add a comment] > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> Patch applied to ath-next branch of ath.git, thanks. b96fab4e3602 ath9k: work around false-positive gcc warning
diff --git a/drivers/net/wireless/ath/ath9k/dynack.c b/drivers/net/wireless/ath/ath9k/dynack.c index fbeb4a739d32..e4eb96b26ca4 100644 --- a/drivers/net/wireless/ath/ath9k/dynack.c +++ b/drivers/net/wireless/ath/ath9k/dynack.c @@ -247,8 +247,14 @@ void ath_dynack_sample_tx_ts(struct ath_hw *ah, struct sk_buff *skb, ridx = ts->ts_rateindex; da->st_rbf.ts[da->st_rbf.t_rb].tstamp = ts->ts_tstamp; +#if defined(CONFIG_KASAN) && (CONFIG_GCC_VERSION >= 100000) && (CONFIG_GCC_VERSION < 110000) + /* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97490 */ + memcpy(da->st_rbf.addr[da->st_rbf.t_rb].h_dest, hdr->addr1, ETH_ALEN); + memcpy(da->st_rbf.addr[da->st_rbf.t_rb].h_src, hdr->addr2, ETH_ALEN); +#else ether_addr_copy(da->st_rbf.addr[da->st_rbf.t_rb].h_dest, hdr->addr1); ether_addr_copy(da->st_rbf.addr[da->st_rbf.t_rb].h_src, hdr->addr2); +#endif if (!(info->status.rates[ridx].flags & IEEE80211_TX_RC_MCS)) { const struct ieee80211_rate *rate;