| Message ID | 20160805203418.32162-1-nicolas.iooss_linux@m4x.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Hello, After I sent the following patch a few weeks ago, I have not received any feedback. Could you please review it and tell me what I may have done wrong? Thanks, Nicolas On 05/08/16 22:34, Nicolas Iooss wrote: > The struct cfg80211_pmksa defines its bssid field as: > > const u8 *bssid; > > contrary to struct brcmf_pmksa, which uses: > > u8 bssid[ETH_ALEN]; > > Therefore in brcmf_cfg80211_del_pmksa(), &pmksa->bssid takes the address > of this field (of type u8**), not the one of its content (which would be > u8*). Remove the & operator to make brcmf_dbg("%pM") and memcmp() > behave as expected. > > This bug have been found using a custom static checker (which checks the > usage of %p... attributes at build time). It has been introduced in > commit 6c404f34f2bd ("brcmfmac: Cleanup pmksa cache handling code"), > which replaced pmksa->bssid by &pmksa->bssid while refactoring the code, > without modifying struct cfg80211_pmksa definition. > > Fixes: 6c404f34f2bd ("brcmfmac: Cleanup pmksa cache handling code") > Cc: stable@ger.kernel.org > Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org> > --- > > scripts/checkpatch.pl reports a warning: "Prefer ether_addr_equal() or > ether_addr_equal_unaligned() over memcmp()". Because some files in > drivers/net/wireless/broadcom/brcm80211/brcmfmac/ still use memcmp() > to compare addresses and because I do not know whether pmksa->bssid is > always aligned, I did not follow this warning. > > drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > index 2628d5e12c64..aceab77cd95a 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > @@ -3884,11 +3884,11 @@ brcmf_cfg80211_del_pmksa(struct wiphy *wiphy, struct net_device *ndev, > if (!check_vif_up(ifp->vif)) > return -EIO; > > - brcmf_dbg(CONN, "del_pmksa - PMK bssid = %pM\n", &pmksa->bssid); > + brcmf_dbg(CONN, "del_pmksa - PMK bssid = %pM\n", pmksa->bssid); > > npmk = le32_to_cpu(cfg->pmk_list.npmk); > for (i = 0; i < npmk; i++) > - if (!memcmp(&pmksa->bssid, &pmk[i].bssid, ETH_ALEN)) > + if (!memcmp(pmksa->bssid, &pmk[i].bssid, ETH_ALEN)) > break; > > if ((npmk > 0) && (i < npmk)) { >
On 22 August 2016 at 15:03, Nicolas Iooss <nicolas.iooss_linux@m4x.org> wrote: > After I sent the following patch a few weeks ago, I have not received > any feedback. Could you please review it and tell me what I may have > done wrong? https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#checking_state_of_patches_from_patchwork https://patchwork.kernel.org/patch/9265733/
On 22-8-2016 15:03, Nicolas Iooss wrote: > Hello, > > After I sent the following patch a few weeks ago, I have not received > any feedback. Could you please review it and tell me what I may have > done wrong? Nothing. People went on vacation :-) > Thanks, > Nicolas > > On 05/08/16 22:34, Nicolas Iooss wrote: >> The struct cfg80211_pmksa defines its bssid field as: >> >> const u8 *bssid; >> >> contrary to struct brcmf_pmksa, which uses: >> >> u8 bssid[ETH_ALEN]; >> >> Therefore in brcmf_cfg80211_del_pmksa(), &pmksa->bssid takes the address >> of this field (of type u8**), not the one of its content (which would be >> u8*). Remove the & operator to make brcmf_dbg("%pM") and memcmp() >> behave as expected. >> >> This bug have been found using a custom static checker (which checks the >> usage of %p... attributes at build time). It has been introduced in >> commit 6c404f34f2bd ("brcmfmac: Cleanup pmksa cache handling code"), >> which replaced pmksa->bssid by &pmksa->bssid while refactoring the code, >> without modifying struct cfg80211_pmksa definition. >> >> Fixes: 6c404f34f2bd ("brcmfmac: Cleanup pmksa cache handling code") >> Cc: stable@ger.kernel.org Ah, so you did something wrong after all :-p. The email address should be 'stable@vger.kernel.org'. >> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org> >> --- >> >> scripts/checkpatch.pl reports a warning: "Prefer ether_addr_equal() or >> ether_addr_equal_unaligned() over memcmp()". Because some files in >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/ still use memcmp() >> to compare addresses and because I do not know whether pmksa->bssid is >> always aligned, I did not follow this warning. As most of this is done in slow path, I prefer memcmp() as I do not want to check alignment for minimal performance gain. >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >> index 2628d5e12c64..aceab77cd95a 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >> @@ -3884,11 +3884,11 @@ brcmf_cfg80211_del_pmksa(struct wiphy *wiphy, struct net_device *ndev, >> if (!check_vif_up(ifp->vif)) >> return -EIO; >> >> - brcmf_dbg(CONN, "del_pmksa - PMK bssid = %pM\n", &pmksa->bssid); >> + brcmf_dbg(CONN, "del_pmksa - PMK bssid = %pM\n", pmksa->bssid); >> >> npmk = le32_to_cpu(cfg->pmk_list.npmk); >> for (i = 0; i < npmk; i++) >> - if (!memcmp(&pmksa->bssid, &pmk[i].bssid, ETH_ALEN)) >> + if (!memcmp(pmksa->bssid, &pmk[i].bssid, ETH_ALEN)) I find '&pmk[i].bssid' confusing so maybe you could change it to '&pmk[i].bssid[0]' or 'pmk[i].bssid' as I think these two are essentially the same. Regards, Arend >> break; >> >> if ((npmk > 0) && (i < npmk)) { >> >
On 22/08/16 21:38, Arend Van Spriel wrote: > On 22-8-2016 15:03, Nicolas Iooss wrote: >> On 05/08/16 22:34, Nicolas Iooss wrote: [...] >>> Fixes: 6c404f34f2bd ("brcmfmac: Cleanup pmksa cache handling code") >>> Cc: stable@ger.kernel.org > > Ah, so you did something wrong after all :-p. The email address should > be 'stable@vger.kernel.org'. Thanks for spotting this! I'll fix this address. >>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org> >>> --- >>> >>> scripts/checkpatch.pl reports a warning: "Prefer ether_addr_equal() or >>> ether_addr_equal_unaligned() over memcmp()". Because some files in >>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/ still use memcmp() >>> to compare addresses and because I do not know whether pmksa->bssid is >>> always aligned, I did not follow this warning. > > As most of this is done in slow path, I prefer memcmp() as I do not want > to check alignment for minimal performance gain. OK. >>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >>> index 2628d5e12c64..aceab77cd95a 100644 >>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >>> @@ -3884,11 +3884,11 @@ brcmf_cfg80211_del_pmksa(struct wiphy *wiphy, struct net_device *ndev, >>> if (!check_vif_up(ifp->vif)) >>> return -EIO; >>> >>> - brcmf_dbg(CONN, "del_pmksa - PMK bssid = %pM\n", &pmksa->bssid); >>> + brcmf_dbg(CONN, "del_pmksa - PMK bssid = %pM\n", pmksa->bssid); >>> >>> npmk = le32_to_cpu(cfg->pmk_list.npmk); >>> for (i = 0; i < npmk; i++) >>> - if (!memcmp(&pmksa->bssid, &pmk[i].bssid, ETH_ALEN)) >>> + if (!memcmp(pmksa->bssid, &pmk[i].bssid, ETH_ALEN)) > > I find '&pmk[i].bssid' confusing so maybe you could change it to > '&pmk[i].bssid[0]' or 'pmk[i].bssid' as I think these two are > essentially the same. I agree the three ways of writing this share the same meaning. I'll send a v2 with 'pmk[i].bssid'. > > Regards, > Arend Thanks for your review! Nicolas
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index 2628d5e12c64..aceab77cd95a 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c @@ -3884,11 +3884,11 @@ brcmf_cfg80211_del_pmksa(struct wiphy *wiphy, struct net_device *ndev, if (!check_vif_up(ifp->vif)) return -EIO; - brcmf_dbg(CONN, "del_pmksa - PMK bssid = %pM\n", &pmksa->bssid); + brcmf_dbg(CONN, "del_pmksa - PMK bssid = %pM\n", pmksa->bssid); npmk = le32_to_cpu(cfg->pmk_list.npmk); for (i = 0; i < npmk; i++) - if (!memcmp(&pmksa->bssid, &pmk[i].bssid, ETH_ALEN)) + if (!memcmp(pmksa->bssid, &pmk[i].bssid, ETH_ALEN)) break; if ((npmk > 0) && (i < npmk)) {
The struct cfg80211_pmksa defines its bssid field as: const u8 *bssid; contrary to struct brcmf_pmksa, which uses: u8 bssid[ETH_ALEN]; Therefore in brcmf_cfg80211_del_pmksa(), &pmksa->bssid takes the address of this field (of type u8**), not the one of its content (which would be u8*). Remove the & operator to make brcmf_dbg("%pM") and memcmp() behave as expected. This bug have been found using a custom static checker (which checks the usage of %p... attributes at build time). It has been introduced in commit 6c404f34f2bd ("brcmfmac: Cleanup pmksa cache handling code"), which replaced pmksa->bssid by &pmksa->bssid while refactoring the code, without modifying struct cfg80211_pmksa definition. Fixes: 6c404f34f2bd ("brcmfmac: Cleanup pmksa cache handling code") Cc: stable@ger.kernel.org Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org> --- scripts/checkpatch.pl reports a warning: "Prefer ether_addr_equal() or ether_addr_equal_unaligned() over memcmp()". Because some files in drivers/net/wireless/broadcom/brcm80211/brcmfmac/ still use memcmp() to compare addresses and because I do not know whether pmksa->bssid is always aligned, I did not follow this warning. drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)