GCC 12 enhanced -Waddress when comparing array address to null [0],
which warns:
drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
drivers/virtio/virtio_pci_common.c:257:29: warning: the comparison will always evaluate as ‘true’ for the pointer operand in ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 256)’ must not be NULL [-Waddress]
257 | if (vp_dev->msix_affinity_masks[i])
| ^~~~~~
In fact, the verification is comparing the result of a pointer
arithmetic, the address "msix_affinity_masks + i", which will always
evaluate to true.
Under the hood, free_cpumask_var() calls kfree(), which is safe to pass
NULL, not requiring non-null verification. So remove the verification
to make compiler happy (happy compiler, happy life).
[0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102103
Signed-off-by: Murilo Opsfelder Araujo <[email protected]>
---
drivers/virtio/virtio_pci_common.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index d724f676608b..5046efcffb4c 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -254,8 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
if (vp_dev->msix_affinity_masks) {
for (i = 0; i < vp_dev->msix_vectors; i++)
- if (vp_dev->msix_affinity_masks[i])
- free_cpumask_var(vp_dev->msix_affinity_masks[i]);
+ free_cpumask_var(vp_dev->msix_affinity_masks[i]);
}
if (vp_dev->msix_enabled) {
--
2.35.1
On 4/14/22 23:30, Murilo Opsfelder Araujo wrote:
> GCC 12 enhanced -Waddress when comparing array address to null [0],
> which warns:
>
> drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
> drivers/virtio/virtio_pci_common.c:257:29: warning: the comparison will always evaluate as ‘true’ for the pointer operand in ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 256)’ must not be NULL [-Waddress]
> 257 | if (vp_dev->msix_affinity_masks[i])
> | ^~~~~~
>
> In fact, the verification is comparing the result of a pointer
> arithmetic, the address "msix_affinity_masks + i", which will always
> evaluate to true.
>
> Under the hood, free_cpumask_var() calls kfree(), which is safe to pass
> NULL, not requiring non-null verification. So remove the verification
> to make compiler happy (happy compiler, happy life).
>
> [0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102103
>
> Signed-off-by: Murilo Opsfelder Araujo <[email protected]>
> ---
> drivers/virtio/virtio_pci_common.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index d724f676608b..5046efcffb4c 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -254,8 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
>
> if (vp_dev->msix_affinity_masks) {
> for (i = 0; i < vp_dev->msix_vectors; i++)
> - if (vp_dev->msix_affinity_masks[i])
> - free_cpumask_var(vp_dev->msix_affinity_masks[i]);
> + free_cpumask_var(vp_dev->msix_affinity_masks[i]);
> }
>
> if (vp_dev->msix_enabled) {
After I sent this message, I realized that Christophe (copied here)
had already proposed a fix:
https://lore.kernel.org/lkml/[email protected]/
Christophe,
Since free_cpumask_var() calls kfree() and kfree() is null-safe,
can we just drop this null verification and call free_cpumask_var() right away?
--
Murilo
> On 28 Apr 2022, at 11:51, Christophe Marie Francois Dupont de Dinechin <[email protected]> wrote:
>
>
>
>> On 28 Apr 2022, at 11:46, Christophe Marie Francois Dupont de Dinechin <[email protected]> wrote:
>>
>>
>>
>>> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <[email protected]> wrote:
>>>
>>> On 4/14/22 23:30, Murilo Opsfelder Araujo wrote:
>>>> GCC 12 enhanced -Waddress when comparing array address to null [0],
>>>> which warns:
>>>> drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
>>>> drivers/virtio/virtio_pci_common.c:257:29: warning: the comparison will always evaluate as ‘true’ for the pointer operand in ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 256)’ must not be NULL [-Waddress]
>>>> 257 | if (vp_dev->msix_affinity_masks[i])
>>>> | ^~~~~~
>>>> In fact, the verification is comparing the result of a pointer
>>>> arithmetic, the address "msix_affinity_masks + i", which will always
>>>> evaluate to true.
>>>> Under the hood, free_cpumask_var() calls kfree(), which is safe to pass
>>>> NULL, not requiring non-null verification. So remove the verification
>>>> to make compiler happy (happy compiler, happy life).
>>>> [0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102103
>>>> Signed-off-by: Murilo Opsfelder Araujo <[email protected]>
>>>> ---
>>>> drivers/virtio/virtio_pci_common.c | 3 +--
>>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>>>> index d724f676608b..5046efcffb4c 100644
>>>> --- a/drivers/virtio/virtio_pci_common.c
>>>> +++ b/drivers/virtio/virtio_pci_common.c
>>>> @@ -254,8 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
>>>> if (vp_dev->msix_affinity_masks) {
>>>> for (i = 0; i < vp_dev->msix_vectors; i++)
>>>> - if (vp_dev->msix_affinity_masks[i])
>>>> - free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>>>> + free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>>>> }
>>>> if (vp_dev->msix_enabled) {
>>>
>>> After I sent this message, I realized that Christophe (copied here)
>>> had already proposed a fix:
>>>
>>> https://lore.kernel.org/lkml/[email protected]/
>>>
>>> Christophe,
>>>
>>> Since free_cpumask_var() calls kfree() and kfree() is null-safe,
>>> can we just drop this null verification and call free_cpumask_var() right away?
>>
>> Apologies for the delay in responding, broken laptop…
>>
>> In the case where CONFIG_CPUMASK_OFFSTACK is not defined, we have:
>>
>> typedef struct cpumask cpumask_var_t[1];
>>
>> So that vp_dev->msix_affinity_masks[i] is statically not null (that’s the warning)
>> but also a static pointer, so not kfree-safe IMO.
>
> … which also renders my own patch invalid :-/
>
> Compiler warnings are good. Clearly not sufficient.
Ah, I just noticed that free_cpumask_var is a noop in that case.
So yes, your fix is better :-)
[Resend, still struggling with new laptop email settings]
> On 28 Apr 2022, at 13:03, Michael S. Tsirkin <[email protected]> wrote:
>
> On Thu, Apr 28, 2022 at 11:55:31AM +0200, Christophe Marie Francois Dupont de Dinechin wrote:
>>
>>
>>> On 28 Apr 2022, at 11:51, Christophe Marie Francois Dupont de Dinechin <[email protected]> wrote:
>>>
>>>
>>>
>>>> On 28 Apr 2022, at 11:46, Christophe Marie Francois Dupont de Dinechin <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>>> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <[email protected]> wrote:
>>>>>
>>>>> On 4/14/22 23:30, Murilo Opsfelder Araujo wrote:
>>>>>> GCC 12 enhanced -Waddress when comparing array address to null [0],
>>>>>> which warns:
>>>>>> drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
>>>>>> drivers/virtio/virtio_pci_common.c:257:29: warning: the comparison will always evaluate as ‘true’ for the pointer operand in ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 256)’ must not be NULL [-Waddress]
>>>>>> 257 | if (vp_dev->msix_affinity_masks[i])
>>>>>> | ^~~~~~
>>>>>> In fact, the verification is comparing the result of a pointer
>>>>>> arithmetic, the address "msix_affinity_masks + i", which will always
>>>>>> evaluate to true.
>>>>>> Under the hood, free_cpumask_var() calls kfree(), which is safe to pass
>>>>>> NULL, not requiring non-null verification. So remove the verification
>>>>>> to make compiler happy (happy compiler, happy life).
>>>>>> [0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102103
>>>>>> Signed-off-by: Murilo Opsfelder Araujo <[email protected]>
>>>>>> ---
>>>>>> drivers/virtio/virtio_pci_common.c | 3 +--
>>>>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>>>>>> index d724f676608b..5046efcffb4c 100644
>>>>>> --- a/drivers/virtio/virtio_pci_common.c
>>>>>> +++ b/drivers/virtio/virtio_pci_common.c
>>>>>> @@ -254,8 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
>>>>>> if (vp_dev->msix_affinity_masks) {
>>>>>> for (i = 0; i < vp_dev->msix_vectors; i++)
>>>>>> - if (vp_dev->msix_affinity_masks[i])
>>>>>> - free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>>>>>> + free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>>>>>> }
>>>>>> if (vp_dev->msix_enabled) {
>>>>>
>>>>> After I sent this message, I realized that Christophe (copied here)
>>>>> had already proposed a fix:
>>>>>
>>>>> https://lore.kernel.org/lkml/[email protected]/
>>>>>
>>>>> Christophe,
>>>>>
>>>>> Since free_cpumask_var() calls kfree() and kfree() is null-safe,
>>>>> can we just drop this null verification and call free_cpumask_var() right away?
>>>>
>>>> Apologies for the delay in responding, broken laptop…
>>>>
>>>> In the case where CONFIG_CPUMASK_OFFSTACK is not defined, we have:
>>>>
>>>> typedef struct cpumask cpumask_var_t[1];
>>>>
>>>> So that vp_dev->msix_affinity_masks[i] is statically not null (that’s the warning)
>>>> but also a static pointer, so not kfree-safe IMO.
>>>
>>> … which also renders my own patch invalid :-/
>>>
>>> Compiler warnings are good. Clearly not sufficient.
>>
>> Ah, I just noticed that free_cpumask_var is a noop in that case.
>>
>> So yes, your fix is better :-)
>
> ACK then?
Yes.
Acked-by: Christophe de Dinechin <[email protected]>
>
> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <[email protected]> wrote:
>
> On 4/14/22 23:30, Murilo Opsfelder Araujo wrote:
>> GCC 12 enhanced -Waddress when comparing array address to null [0],
>> which warns:
>> drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
>> drivers/virtio/virtio_pci_common.c:257:29: warning: the comparison will always evaluate as ‘true’ for the pointer operand in ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 256)’ must not be NULL [-Waddress]
>> 257 | if (vp_dev->msix_affinity_masks[i])
>> | ^~~~~~
>> In fact, the verification is comparing the result of a pointer
>> arithmetic, the address "msix_affinity_masks + i", which will always
>> evaluate to true.
>> Under the hood, free_cpumask_var() calls kfree(), which is safe to pass
>> NULL, not requiring non-null verification. So remove the verification
>> to make compiler happy (happy compiler, happy life).
>> [0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102103
>> Signed-off-by: Murilo Opsfelder Araujo <[email protected]>
>> ---
>> drivers/virtio/virtio_pci_common.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>> index d724f676608b..5046efcffb4c 100644
>> --- a/drivers/virtio/virtio_pci_common.c
>> +++ b/drivers/virtio/virtio_pci_common.c
>> @@ -254,8 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
>> if (vp_dev->msix_affinity_masks) {
>> for (i = 0; i < vp_dev->msix_vectors; i++)
>> - if (vp_dev->msix_affinity_masks[i])
>> - free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>> + free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>> }
>> if (vp_dev->msix_enabled) {
>
> After I sent this message, I realized that Christophe (copied here)
> had already proposed a fix:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> Christophe,
>
> Since free_cpumask_var() calls kfree() and kfree() is null-safe,
> can we just drop this null verification and call free_cpumask_var() right away?
Apologies for the delay in responding, broken laptop…
In the case where CONFIG_CPUMASK_OFFSTACK is not defined, we have:
typedef struct cpumask cpumask_var_t[1];
So that vp_dev->msix_affinity_masks[i] is statically not null (that’s the warning)
but also a static pointer, so not kfree-safe IMO.
On Thu, Apr 28, 2022 at 11:55:31AM +0200, Christophe Marie Francois Dupont de Dinechin wrote:
>
>
> > On 28 Apr 2022, at 11:51, Christophe Marie Francois Dupont de Dinechin <[email protected]> wrote:
> >
> >
> >
> >> On 28 Apr 2022, at 11:46, Christophe Marie Francois Dupont de Dinechin <[email protected]> wrote:
> >>
> >>
> >>
> >>> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <[email protected]> wrote:
> >>>
> >>> On 4/14/22 23:30, Murilo Opsfelder Araujo wrote:
> >>>> GCC 12 enhanced -Waddress when comparing array address to null [0],
> >>>> which warns:
> >>>> drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
> >>>> drivers/virtio/virtio_pci_common.c:257:29: warning: the comparison will always evaluate as ‘true’ for the pointer operand in ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 256)’ must not be NULL [-Waddress]
> >>>> 257 | if (vp_dev->msix_affinity_masks[i])
> >>>> | ^~~~~~
> >>>> In fact, the verification is comparing the result of a pointer
> >>>> arithmetic, the address "msix_affinity_masks + i", which will always
> >>>> evaluate to true.
> >>>> Under the hood, free_cpumask_var() calls kfree(), which is safe to pass
> >>>> NULL, not requiring non-null verification. So remove the verification
> >>>> to make compiler happy (happy compiler, happy life).
> >>>> [0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102103
> >>>> Signed-off-by: Murilo Opsfelder Araujo <[email protected]>
> >>>> ---
> >>>> drivers/virtio/virtio_pci_common.c | 3 +--
> >>>> 1 file changed, 1 insertion(+), 2 deletions(-)
> >>>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> >>>> index d724f676608b..5046efcffb4c 100644
> >>>> --- a/drivers/virtio/virtio_pci_common.c
> >>>> +++ b/drivers/virtio/virtio_pci_common.c
> >>>> @@ -254,8 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
> >>>> if (vp_dev->msix_affinity_masks) {
> >>>> for (i = 0; i < vp_dev->msix_vectors; i++)
> >>>> - if (vp_dev->msix_affinity_masks[i])
> >>>> - free_cpumask_var(vp_dev->msix_affinity_masks[i]);
> >>>> + free_cpumask_var(vp_dev->msix_affinity_masks[i]);
> >>>> }
> >>>> if (vp_dev->msix_enabled) {
> >>>
> >>> After I sent this message, I realized that Christophe (copied here)
> >>> had already proposed a fix:
> >>>
> >>> https://lore.kernel.org/lkml/[email protected]/
> >>>
> >>> Christophe,
> >>>
> >>> Since free_cpumask_var() calls kfree() and kfree() is null-safe,
> >>> can we just drop this null verification and call free_cpumask_var() right away?
> >>
> >> Apologies for the delay in responding, broken laptop…
> >>
> >> In the case where CONFIG_CPUMASK_OFFSTACK is not defined, we have:
> >>
> >> typedef struct cpumask cpumask_var_t[1];
> >>
> >> So that vp_dev->msix_affinity_masks[i] is statically not null (that’s the warning)
> >> but also a static pointer, so not kfree-safe IMO.
> >
> > … which also renders my own patch invalid :-/
> >
> > Compiler warnings are good. Clearly not sufficient.
>
> Ah, I just noticed that free_cpumask_var is a noop in that case.
>
> So yes, your fix is better :-)
ACK then?
> On 28 Apr 2022, at 11:46, Christophe Marie Francois Dupont de Dinechin <[email protected]> wrote:
>
>
>
>> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <[email protected]> wrote:
>>
>> On 4/14/22 23:30, Murilo Opsfelder Araujo wrote:
>>> GCC 12 enhanced -Waddress when comparing array address to null [0],
>>> which warns:
>>> drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
>>> drivers/virtio/virtio_pci_common.c:257:29: warning: the comparison will always evaluate as ‘true’ for the pointer operand in ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 256)’ must not be NULL [-Waddress]
>>> 257 | if (vp_dev->msix_affinity_masks[i])
>>> | ^~~~~~
>>> In fact, the verification is comparing the result of a pointer
>>> arithmetic, the address "msix_affinity_masks + i", which will always
>>> evaluate to true.
>>> Under the hood, free_cpumask_var() calls kfree(), which is safe to pass
>>> NULL, not requiring non-null verification. So remove the verification
>>> to make compiler happy (happy compiler, happy life).
>>> [0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102103
>>> Signed-off-by: Murilo Opsfelder Araujo <[email protected]>
>>> ---
>>> drivers/virtio/virtio_pci_common.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>>> index d724f676608b..5046efcffb4c 100644
>>> --- a/drivers/virtio/virtio_pci_common.c
>>> +++ b/drivers/virtio/virtio_pci_common.c
>>> @@ -254,8 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
>>> if (vp_dev->msix_affinity_masks) {
>>> for (i = 0; i < vp_dev->msix_vectors; i++)
>>> - if (vp_dev->msix_affinity_masks[i])
>>> - free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>>> + free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>>> }
>>> if (vp_dev->msix_enabled) {
>>
>> After I sent this message, I realized that Christophe (copied here)
>> had already proposed a fix:
>>
>> https://lore.kernel.org/lkml/[email protected]/
>>
>> Christophe,
>>
>> Since free_cpumask_var() calls kfree() and kfree() is null-safe,
>> can we just drop this null verification and call free_cpumask_var() right away?
>
> Apologies for the delay in responding, broken laptop…
>
> In the case where CONFIG_CPUMASK_OFFSTACK is not defined, we have:
>
> typedef struct cpumask cpumask_var_t[1];
>
> So that vp_dev->msix_affinity_masks[i] is statically not null (that’s the warning)
> but also a static pointer, so not kfree-safe IMO.
… which also renders my own patch invalid :-/
Compiler warnings are good. Clearly not sufficient.