From: Colin Ian King <[email protected]>
The pointer rtwsta is dereferencing pointer sta before sta is
being null checked, so there is a potential null pointer deference
issue that may occur. Fix this by only assigning rtwsta after sta
has been null checked. Add in a null pointer check on rtwsta before
dereferencing it too.
Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
Addresses-Coverity: ("Dereference before null check")
Signed-off-by: Colin Ian King <[email protected]>
---
drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 06fb6e5b1b37..26f52a25f545 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
{
struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
struct ieee80211_sta *sta = txq->sta;
- struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
+ struct rtw89_sta *rtwsta;
- if (!sta || rtwsta->max_agg_wait <= 0)
+ if (!sta)
+ return false;
+ rtwsta = (struct rtw89_sta *)sta->drv_priv;
+ if (!rtwsta)
+ return false;
+ if (rtwsta->max_agg_wait <= 0)
return false;
if (rtwdev->stats.tx_tfc_lv <= RTW89_TFC_MID)
--
2.32.0
Pkshih <[email protected]> writes:
>> -----Original Message-----
>> From: Colin King <[email protected]>
>> Sent: Friday, October 15, 2021 11:46 PM
>> To: Kalle Valo <[email protected]>; David S . Miller <[email protected]>; Jakub Kicinski
>> <[email protected]>; Pkshih <[email protected]>; [email protected];
>> [email protected]
>> Cc: [email protected]; [email protected]
>> Subject: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
>>
>> From: Colin Ian King <[email protected]>
>>
>> The pointer rtwsta is dereferencing pointer sta before sta is
>> being null checked, so there is a potential null pointer deference
>> issue that may occur. Fix this by only assigning rtwsta after sta
>> has been null checked. Add in a null pointer check on rtwsta before
>> dereferencing it too.
>>
>> Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
>> Addresses-Coverity: ("Dereference before null check")
>> Signed-off-by: Colin Ian King <[email protected]>
>> ---
>> drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtw89/core.c
>> b/drivers/net/wireless/realtek/rtw89/core.c
>> index 06fb6e5b1b37..26f52a25f545 100644
>> --- a/drivers/net/wireless/realtek/rtw89/core.c
>> +++ b/drivers/net/wireless/realtek/rtw89/core.c
>> @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
>> {
>> struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
>> struct ieee80211_sta *sta = txq->sta;
>> - struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
>
> 'sta->drv_priv' is only a pointer, we don't really dereference the
> data right here, so I think this is safe. More, compiler can optimize
> this instruction that reorder it to the place just right before using.
> So, it seems like a false alarm.
>
>> + struct rtw89_sta *rtwsta;
>>
>> - if (!sta || rtwsta->max_agg_wait <= 0)
>> + if (!sta)
>> + return false;
>> + rtwsta = (struct rtw89_sta *)sta->drv_priv;
>> + if (!rtwsta)
>> + return false;
>> + if (rtwsta->max_agg_wait <= 0)
>> return false;
>>
>> if (rtwdev->stats.tx_tfc_lv <= RTW89_TFC_MID)
>
> I check the size of object files before/after this patch, and
> the original one is smaller.
>
> text data bss dec hex filename
> 16781 3392 1 20174 4ece core-0.o // original
> 16819 3392 1 20212 4ef4 core-1.o // after this patch
>
> Do you think it is worth to apply this patch?
I think that we should apply the patch. Even though the compiler _may_
reorder the code, it might choose not to do that.
Another question is that can txq->sta really be null? I didn't check the
code, but if it should be always set when the null check is not needed.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> -----Original Message-----
> From: [email protected] <[email protected]> On
> Behalf Of Kalle Valo
> Sent: Monday, October 18, 2021 8:12 PM
> To: Pkshih <[email protected]>
> Cc: Colin King <[email protected]>; David S . Miller <[email protected]>; Jakub
> Kicinski <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
>
> Pkshih <[email protected]> writes:
>
> >> -----Original Message-----
> >> From: Colin King <[email protected]>
> >> Sent: Friday, October 15, 2021 11:46 PM
> >> To: Kalle Valo <[email protected]>; David S . Miller <[email protected]>; Jakub Kicinski
> >> <[email protected]>; Pkshih <[email protected]>; [email protected];
> >> [email protected]
> >> Cc: [email protected]; [email protected]
> >> Subject: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> >>
> >> From: Colin Ian King <[email protected]>
> >>
> >> The pointer rtwsta is dereferencing pointer sta before sta is
> >> being null checked, so there is a potential null pointer deference
> >> issue that may occur. Fix this by only assigning rtwsta after sta
> >> has been null checked. Add in a null pointer check on rtwsta before
> >> dereferencing it too.
> >>
> >> Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
> >> Addresses-Coverity: ("Dereference before null check")
> >> Signed-off-by: Colin Ian King <[email protected]>
> >> ---
> >> drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
> >> 1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> >> b/drivers/net/wireless/realtek/rtw89/core.c
> >> index 06fb6e5b1b37..26f52a25f545 100644
> >> --- a/drivers/net/wireless/realtek/rtw89/core.c
> >> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> >> @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> >> {
> >> struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> >> struct ieee80211_sta *sta = txq->sta;
> >> - struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> >
> > 'sta->drv_priv' is only a pointer, we don't really dereference the
> > data right here, so I think this is safe. More, compiler can optimize
> > this instruction that reorder it to the place just right before using.
> > So, it seems like a false alarm.
> >
> >> + struct rtw89_sta *rtwsta;
> >>
> >> - if (!sta || rtwsta->max_agg_wait <= 0)
> >> + if (!sta)
> >> + return false;
> >> + rtwsta = (struct rtw89_sta *)sta->drv_priv;
> >> + if (!rtwsta)
> >> + return false;
> >> + if (rtwsta->max_agg_wait <= 0)
> >> return false;
> >>
> >> if (rtwdev->stats.tx_tfc_lv <= RTW89_TFC_MID)
> >
> > I check the size of object files before/after this patch, and
> > the original one is smaller.
> >
> > text data bss dec hex filename
> > 16781 3392 1 20174 4ece core-0.o // original
> > 16819 3392 1 20212 4ef4 core-1.o // after this patch
> >
> > Do you think it is worth to apply this patch?
>
> I think that we should apply the patch. Even though the compiler _may_
> reorder the code, it might choose not to do that.
Understand.
I have another way to fix this coverity warning, like:
@@ -1617,7 +1617,7 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
{
struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
struct ieee80211_sta *sta = txq->sta;
- struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
+ struct rtw89_sta *rtwsta = sta ? (struct rtw89_sta *)sta->drv_priv : NULL;
if (!sta || rtwsta->max_agg_wait <= 0)
return false;
Is this acceptable?
It has a little redundant checking of 'sta', but the code looks clean.
>
> Another question is that can txq->sta really be null? I didn't check the
> code, but if it should be always set when the null check is not needed.
>
It says
* struct ieee80211_txq - Software intermediate tx queue
* @sta: station table entry, %NULL for per-vif queue
So, we need to check if 'sta' is NULL.
--
Ping-Ke
> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf Of Kalle
> Valo
> Sent: Wednesday, October 20, 2021 4:36 PM
> To: Pkshih <[email protected]>
> Cc: Colin King <[email protected]>; David S . Miller <[email protected]>; Jakub Kicinski
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
>
> Pkshih <[email protected]> writes:
>
> >> -----Original Message-----
> >> From: [email protected]
> >> <[email protected]> On
> >> Behalf Of Kalle Valo
> >> Sent: Monday, October 18, 2021 8:12 PM
> >> To: Pkshih <[email protected]>
> >> Cc: Colin King <[email protected]>; David S . Miller
> >> <[email protected]>; Jakub
> >> Kicinski <[email protected]>; [email protected]; [email protected];
> >> [email protected]; [email protected]
> >> Subject: Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> >>
> >> Pkshih <[email protected]> writes:
> >>
> >> >> -----Original Message-----
> >> >> From: Colin King <[email protected]>
> >> >> Sent: Friday, October 15, 2021 11:46 PM
> >> >> To: Kalle Valo <[email protected]>; David S . Miller <[email protected]>; Jakub Kicinski
> >> >> <[email protected]>; Pkshih <[email protected]>; [email protected];
> >> >> [email protected]
> >> >> Cc: [email protected]; [email protected]
> >> >> Subject: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> >> >>
> >> >> From: Colin Ian King <[email protected]>
> >> >>
> >> >> The pointer rtwsta is dereferencing pointer sta before sta is
> >> >> being null checked, so there is a potential null pointer deference
> >> >> issue that may occur. Fix this by only assigning rtwsta after sta
> >> >> has been null checked. Add in a null pointer check on rtwsta before
> >> >> dereferencing it too.
> >> >>
> >> >> Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
> >> >> Addresses-Coverity: ("Dereference before null check")
> >> >> Signed-off-by: Colin Ian King <[email protected]>
> >> >> ---
> >> >> drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
> >> >> 1 file changed, 7 insertions(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> >> >> b/drivers/net/wireless/realtek/rtw89/core.c
> >> >> index 06fb6e5b1b37..26f52a25f545 100644
> >> >> --- a/drivers/net/wireless/realtek/rtw89/core.c
> >> >> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> >> >> @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> >> >> {
> >> >> struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> >> >> struct ieee80211_sta *sta = txq->sta;
> >> >> - struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> >> >
> >> > 'sta->drv_priv' is only a pointer, we don't really dereference the
> >> > data right here, so I think this is safe. More, compiler can optimize
> >> > this instruction that reorder it to the place just right before using.
> >> > So, it seems like a false alarm.
> >> >
> >> >> + struct rtw89_sta *rtwsta;
> >> >>
> >> >> - if (!sta || rtwsta->max_agg_wait <= 0)
> >> >> + if (!sta)
> >> >> + return false;
> >> >> + rtwsta = (struct rtw89_sta *)sta->drv_priv;
> >> >> + if (!rtwsta)
> >> >> + return false;
> >> >> + if (rtwsta->max_agg_wait <= 0)
> >> >> return false;
> >> >>
> >> >> if (rtwdev->stats.tx_tfc_lv <= RTW89_TFC_MID)
> >> >
> >> > I check the size of object files before/after this patch, and
> >> > the original one is smaller.
> >> >
> >> > text data bss dec hex filename
> >> > 16781 3392 1 20174 4ece core-0.o // original
> >> > 16819 3392 1 20212 4ef4 core-1.o // after this patch
> >> >
> >> > Do you think it is worth to apply this patch?
> >>
> >> I think that we should apply the patch. Even though the compiler _may_
> >> reorder the code, it might choose not to do that.
> >
> > Understand.
> >
> > I have another way to fix this coverity warning, like:
> >
> > @@ -1617,7 +1617,7 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> > {
> > struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> > struct ieee80211_sta *sta = txq->sta;
> > - struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> > + struct rtw89_sta *rtwsta = sta ? (struct rtw89_sta *)sta->drv_priv : NULL;
> >
> > if (!sta || rtwsta->max_agg_wait <= 0)
> > return false;
> >
> > Is this acceptable?
> > It has a little redundant checking of 'sta', but the code looks clean.
>
> I feel that Colin's fix is more readable, but this is just matter of
> taste. You can choose.
I would like my version.
There are three similar warnings reported by smatch, so I will fix them by
myself. Please drop this patch.
But, still thank Colin to point out this issue.
--
Ping-Ke
Pkshih <[email protected]> writes:
>> >> > I check the size of object files before/after this patch, and
>> >> > the original one is smaller.
>> >> >
>> >> > text data bss dec hex filename
>> >> > 16781 3392 1 20174 4ece core-0.o // original
>> >> > 16819 3392 1 20212 4ef4 core-1.o // after this patch
>> >> >
>> >> > Do you think it is worth to apply this patch?
>> >>
>> >> I think that we should apply the patch. Even though the compiler _may_
>> >> reorder the code, it might choose not to do that.
>> >
>> > Understand.
>> >
>> > I have another way to fix this coverity warning, like:
>> >
>> > @@ -1617,7 +1617,7 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
>> > {
>> > struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
>> > struct ieee80211_sta *sta = txq->sta;
>> > - struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
>> > + struct rtw89_sta *rtwsta = sta ? (struct rtw89_sta *)sta->drv_priv : NULL;
>> >
>> > if (!sta || rtwsta->max_agg_wait <= 0)
>> > return false;
>> >
>> > Is this acceptable?
>> > It has a little redundant checking of 'sta', but the code looks clean.
>>
>> I feel that Colin's fix is more readable, but this is just matter of
>> taste. You can choose.
>
> I would like my version.
>
> There are three similar warnings reported by smatch, so I will fix them by
> myself. Please drop this patch.
Ok, dropped.
> But, still thank Colin to point out this issue.
Indeed, thanks Colin. A good way to thank is to add Reported-by to the
commit log.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Mon, Oct 18, 2021 at 03:35:28AM +0000, Pkshih wrote:
>
> > -----Original Message-----
> > From: Colin King <[email protected]>
> > Sent: Friday, October 15, 2021 11:46 PM
> > To: Kalle Valo <[email protected]>; David S . Miller <[email protected]>; Jakub Kicinski
> > <[email protected]>; Pkshih <[email protected]>; [email protected];
> > [email protected]
> > Cc: [email protected]; [email protected]
> > Subject: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> >
> > From: Colin Ian King <[email protected]>
> >
> > The pointer rtwsta is dereferencing pointer sta before sta is
> > being null checked, so there is a potential null pointer deference
> > issue that may occur. Fix this by only assigning rtwsta after sta
> > has been null checked. Add in a null pointer check on rtwsta before
> > dereferencing it too.
> >
> > Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
> > Addresses-Coverity: ("Dereference before null check")
> > Signed-off-by: Colin Ian King <[email protected]>
> > ---
> > drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> > b/drivers/net/wireless/realtek/rtw89/core.c
> > index 06fb6e5b1b37..26f52a25f545 100644
> > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> > {
> > struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> > struct ieee80211_sta *sta = txq->sta;
> > - struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
>
> 'sta->drv_priv' is only a pointer, we don't really dereference the
> data right here, so I think this is safe. More, compiler can optimize
> this instruction that reorder it to the place just right before using.
> So, it seems like a false alarm.
The warning is about "sta" not "sta->priv". It's not a false positive.
I have heard discussions about compilers trying to work around these
bugs by re-ordering the code. Is that an option in GCC? It's not
something we should rely on, but I'm just curious if it exists in
released versions.
regards,
dan carpenter
> -----Original Message-----
> From: Dan Carpenter <[email protected]>
> Sent: Tuesday, November 2, 2021 9:15 PM
> To: Pkshih <[email protected]>
> Cc: Colin King <[email protected]>; Kalle Valo <[email protected]>; David S . Miller
> <[email protected]>; Jakub Kicinski <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
>
> On Mon, Oct 18, 2021 at 03:35:28AM +0000, Pkshih wrote:
> >
> > > -----Original Message-----
> > > From: Colin King <[email protected]>
> > > Sent: Friday, October 15, 2021 11:46 PM
> > > To: Kalle Valo <[email protected]>; David S . Miller <[email protected]>; Jakub Kicinski
> > > <[email protected]>; Pkshih <[email protected]>; [email protected];
> > > [email protected]
> > > Cc: [email protected]; [email protected]
> > > Subject: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> > >
> > > From: Colin Ian King <[email protected]>
> > >
> > > The pointer rtwsta is dereferencing pointer sta before sta is
> > > being null checked, so there is a potential null pointer deference
> > > issue that may occur. Fix this by only assigning rtwsta after sta
> > > has been null checked. Add in a null pointer check on rtwsta before
> > > dereferencing it too.
> > >
> > > Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
> > > Addresses-Coverity: ("Dereference before null check")
> > > Signed-off-by: Colin Ian King <[email protected]>
> > > ---
> > > drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> > > b/drivers/net/wireless/realtek/rtw89/core.c
> > > index 06fb6e5b1b37..26f52a25f545 100644
> > > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > > @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> > > {
> > > struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> > > struct ieee80211_sta *sta = txq->sta;
> > > - struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> >
> > 'sta->drv_priv' is only a pointer, we don't really dereference the
> > data right here, so I think this is safe. More, compiler can optimize
> > this instruction that reorder it to the place just right before using.
> > So, it seems like a false alarm.
>
> The warning is about "sta" not "sta->priv". It's not a false positive.
>
> I have heard discussions about compilers trying to work around these
> bugs by re-ordering the code. Is that an option in GCC? It's not
> something we should rely on, but I'm just curious if it exists in
> released versions.
>
I say GCC does "reorder" the code, because the object codes of following
two codes are identical with default or -Os ccflags.
If I misuse the term, please correct me.
Code-1:
struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
if (!sta)
return false;
if (rtwsta->max_agg_wait <= 0)
return false;
Code-2:
struct rtw89_sta *rtwsta;
if (!sta)
return false;
rtwsta = (struct rtw89_sta *)sta->drv_priv;
if (rtwsta->max_agg_wait <= 0)
return false;
The code-1 is the original code Coverity and smatch warn use-before-check.
The code-2 can avoid this warning without doubt.
To be clear, I have sent a patch to fix this.
--
Ping-Ke
On Wed, Nov 03, 2021 at 12:36:17AM +0000, Pkshih wrote:
> > > > diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> > > > b/drivers/net/wireless/realtek/rtw89/core.c
> > > > index 06fb6e5b1b37..26f52a25f545 100644
> > > > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > > > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > > > @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> > > > {
> > > > struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> > > > struct ieee80211_sta *sta = txq->sta;
> > > > - struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> > >
> > > 'sta->drv_priv' is only a pointer, we don't really dereference the
> > > data right here, so I think this is safe. More, compiler can optimize
> > > this instruction that reorder it to the place just right before using.
> > > So, it seems like a false alarm.
> >
> > The warning is about "sta" not "sta->priv". It's not a false positive.
> >
> > I have heard discussions about compilers trying to work around these
> > bugs by re-ordering the code. Is that an option in GCC? It's not
> > something we should rely on, but I'm just curious if it exists in
> > released versions.
> >
>
> I say GCC does "reorder" the code, because the object codes of following
> two codes are identical with default or -Os ccflags.
Huh... That's cool. GCC doesn't re-order it for me, but I'm on GCC 8
so maybe it will work when I get to a more modern version.
regards,
dan carpenter
> -----Original Message-----
> From: Dan Carpenter <[email protected]>
> Sent: Wednesday, November 3, 2021 6:21 PM
> To: Pkshih <[email protected]>
> Cc: Colin King <[email protected]>; Kalle Valo <[email protected]>; David S . Miller
> <[email protected]>; Jakub Kicinski <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
>
> On Wed, Nov 03, 2021 at 12:36:17AM +0000, Pkshih wrote:
>
> > > > > diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> > > > > b/drivers/net/wireless/realtek/rtw89/core.c
> > > > > index 06fb6e5b1b37..26f52a25f545 100644
> > > > > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > > > > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > > > > @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> > > > > {
> > > > > struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> > > > > struct ieee80211_sta *sta = txq->sta;
> > > > > - struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> > > >
> > > > 'sta->drv_priv' is only a pointer, we don't really dereference the
> > > > data right here, so I think this is safe. More, compiler can optimize
> > > > this instruction that reorder it to the place just right before using.
> > > > So, it seems like a false alarm.
> > >
> > > The warning is about "sta" not "sta->priv". It's not a false positive.
> > >
> > > I have heard discussions about compilers trying to work around these
> > > bugs by re-ordering the code. Is that an option in GCC? It's not
> > > something we should rely on, but I'm just curious if it exists in
> > > released versions.
> > >
> >
> > I say GCC does "reorder" the code, because the object codes of following
> > two codes are identical with default or -Os ccflags.
>
> Huh... That's cool. GCC doesn't re-order it for me, but I'm on GCC 8
> so maybe it will work when I get to a more modern version.
>
My GCC is 9.3.0.
But, I don't try other versions.
--
Ping-Ke