* [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs()
@ 2022-04-15 2:30 Murilo Opsfelder Araujo
2022-04-15 3:51 ` Murilo Opsfelder Araújo
0 siblings, 1 reply; 13+ messages in thread
From: Murilo Opsfelder Araujo @ 2022-04-15 2:30 UTC (permalink / raw)
To: mst
Cc: jasowang, virtualization, linux-kernel, mopsfelder,
Murilo Opsfelder Araujo
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 <muriloo@linux.ibm.com>
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs()
2022-04-15 2:30 [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs() Murilo Opsfelder Araujo
@ 2022-04-15 3:51 ` Murilo Opsfelder Araújo
2022-04-28 9:46 ` Christophe Marie Francois Dupont de Dinechin
0 siblings, 1 reply; 13+ messages in thread
From: Murilo Opsfelder Araújo @ 2022-04-15 3:51 UTC (permalink / raw)
To: mst; +Cc: jasowang, virtualization, linux-kernel, mopsfelder, dinechin
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 <muriloo@linux.ibm.com>
> ---
> 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/20220414150855.2407137-4-dinechin@redhat.com/
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs()
2022-04-15 3:51 ` Murilo Opsfelder Araújo
@ 2022-04-28 9:46 ` Christophe Marie Francois Dupont de Dinechin
0 siblings, 0 replies; 13+ messages in thread
From: Christophe Marie Francois Dupont de Dinechin @ 2022-04-28 9:46 UTC (permalink / raw)
To: muriloo
Cc: Michael S. Tsirkin, linux-kernel, virtualization, mopsfelder,
Christophe de Dinechin
> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <muriloo@linux.ibm.com> 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 <muriloo@linux.ibm.com>
>> ---
>> 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/20220414150855.2407137-4-dinechin@redhat.com/
>
> 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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs()
@ 2022-04-28 9:46 ` Christophe Marie Francois Dupont de Dinechin
0 siblings, 0 replies; 13+ messages in thread
From: Christophe Marie Francois Dupont de Dinechin @ 2022-04-28 9:46 UTC (permalink / raw)
To: muriloo
Cc: Michael S. Tsirkin, Jason Wang, virtualization, linux-kernel,
mopsfelder, Christophe de Dinechin
> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <muriloo@linux.ibm.com> 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 <muriloo@linux.ibm.com>
>> ---
>> 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/20220414150855.2407137-4-dinechin@redhat.com/
>
> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs()
2022-04-28 9:46 ` Christophe Marie Francois Dupont de Dinechin
@ 2022-04-28 9:51 ` Christophe Marie Francois Dupont de Dinechin
-1 siblings, 0 replies; 13+ messages in thread
From: Christophe Marie Francois Dupont de Dinechin @ 2022-04-28 9:51 UTC (permalink / raw)
To: muriloo
Cc: Michael S. Tsirkin, linux-kernel, virtualization, mopsfelder,
Christophe de Dinechin
> On 28 Apr 2022, at 11:46, Christophe Marie Francois Dupont de Dinechin <cdupontd@redhat.com> wrote:
>
>
>
>> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <muriloo@linux.ibm.com> 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 <muriloo@linux.ibm.com>
>>> ---
>>> 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/20220414150855.2407137-4-dinechin@redhat.com/
>>
>> 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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs()
@ 2022-04-28 9:51 ` Christophe Marie Francois Dupont de Dinechin
0 siblings, 0 replies; 13+ messages in thread
From: Christophe Marie Francois Dupont de Dinechin @ 2022-04-28 9:51 UTC (permalink / raw)
To: muriloo
Cc: Michael S. Tsirkin, Jason Wang, virtualization, linux-kernel,
mopsfelder, Christophe de Dinechin
> On 28 Apr 2022, at 11:46, Christophe Marie Francois Dupont de Dinechin <cdupontd@redhat.com> wrote:
>
>
>
>> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <muriloo@linux.ibm.com> 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 <muriloo@linux.ibm.com>
>>> ---
>>> 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/20220414150855.2407137-4-dinechin@redhat.com/
>>
>> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs()
2022-04-28 9:51 ` Christophe Marie Francois Dupont de Dinechin
@ 2022-04-28 9:55 ` Christophe Marie Francois Dupont de Dinechin
-1 siblings, 0 replies; 13+ messages in thread
From: Christophe Marie Francois Dupont de Dinechin @ 2022-04-28 9:55 UTC (permalink / raw)
To: Murilo Opsfelder Araujo
Cc: Michael S. Tsirkin, linux-kernel, virtualization, mopsfelder,
Christophe de Dinechin
> On 28 Apr 2022, at 11:51, Christophe Marie Francois Dupont de Dinechin <cdupontd@redhat.com> wrote:
>
>
>
>> On 28 Apr 2022, at 11:46, Christophe Marie Francois Dupont de Dinechin <cdupontd@redhat.com> wrote:
>>
>>
>>
>>> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <muriloo@linux.ibm.com> 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 <muriloo@linux.ibm.com>
>>>> ---
>>>> 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/20220414150855.2407137-4-dinechin@redhat.com/
>>>
>>> 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 :-)
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs()
@ 2022-04-28 9:55 ` Christophe Marie Francois Dupont de Dinechin
0 siblings, 0 replies; 13+ messages in thread
From: Christophe Marie Francois Dupont de Dinechin @ 2022-04-28 9:55 UTC (permalink / raw)
To: Murilo Opsfelder Araujo
Cc: Michael S. Tsirkin, Jason Wang, virtualization, linux-kernel,
mopsfelder, Christophe de Dinechin
> On 28 Apr 2022, at 11:51, Christophe Marie Francois Dupont de Dinechin <cdupontd@redhat.com> wrote:
>
>
>
>> On 28 Apr 2022, at 11:46, Christophe Marie Francois Dupont de Dinechin <cdupontd@redhat.com> wrote:
>>
>>
>>
>>> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <muriloo@linux.ibm.com> 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 <muriloo@linux.ibm.com>
>>>> ---
>>>> 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/20220414150855.2407137-4-dinechin@redhat.com/
>>>
>>> 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 :-)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs()
2022-04-28 9:55 ` Christophe Marie Francois Dupont de Dinechin
@ 2022-04-28 11:03 ` Michael S. Tsirkin
-1 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2022-04-28 11:03 UTC (permalink / raw)
To: Christophe Marie Francois Dupont de Dinechin
Cc: Murilo Opsfelder Araujo, Jason Wang, virtualization,
linux-kernel, mopsfelder, Christophe de Dinechin
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 <cdupontd@redhat.com> wrote:
> >
> >
> >
> >> On 28 Apr 2022, at 11:46, Christophe Marie Francois Dupont de Dinechin <cdupontd@redhat.com> wrote:
> >>
> >>
> >>
> >>> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <muriloo@linux.ibm.com> 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 <muriloo@linux.ibm.com>
> >>>> ---
> >>>> 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/20220414150855.2407137-4-dinechin@redhat.com/
> >>>
> >>> 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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs()
@ 2022-04-28 11:03 ` Michael S. Tsirkin
0 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2022-04-28 11:03 UTC (permalink / raw)
To: Christophe Marie Francois Dupont de Dinechin
Cc: linux-kernel, virtualization, mopsfelder,
Murilo Opsfelder Araujo, Christophe de Dinechin
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 <cdupontd@redhat.com> wrote:
> >
> >
> >
> >> On 28 Apr 2022, at 11:46, Christophe Marie Francois Dupont de Dinechin <cdupontd@redhat.com> wrote:
> >>
> >>
> >>
> >>> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <muriloo@linux.ibm.com> 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 <muriloo@linux.ibm.com>
> >>>> ---
> >>>> 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/20220414150855.2407137-4-dinechin@redhat.com/
> >>>
> >>> 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?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs()
2022-04-28 11:03 ` Michael S. Tsirkin
(?)
@ 2022-04-28 11:43 ` Christophe Marie Francois Dupont de Dinechin
-1 siblings, 0 replies; 13+ messages in thread
From: Christophe Marie Francois Dupont de Dinechin @ 2022-04-28 11:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, virtualization, mopsfelder,
Murilo Opsfelder Araujo, Christophe de Dinechin
[-- Attachment #1.1: Type: text/plain, Size: 3460 bytes --]
> On 28 Apr 2022, at 13:03, Michael S. Tsirkin <mst@redhat.com> 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 <cdupontd@redhat.com> wrote:
>>>
>>>
>>>
>>>> On 28 Apr 2022, at 11:46, Christophe Marie Francois Dupont de Dinechin <cdupontd@redhat.com> wrote:
>>>>
>>>>
>>>>
>>>>> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <muriloo@linux.ibm.com> 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 <muriloo@linux.ibm.com>
>>>>>> ---
>>>>>> 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/20220414150855.2407137-4-dinechin@redhat.com/
>>>>>
>>>>> 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 <dinechin@redhat.com <mailto:dinechin@redhat.com>>
>
[-- Attachment #1.2: Type: text/html, Size: 6100 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs()
2022-04-28 11:03 ` Michael S. Tsirkin
@ 2022-04-28 11:52 ` Christophe Marie Francois Dupont de Dinechin
-1 siblings, 0 replies; 13+ messages in thread
From: Christophe Marie Francois Dupont de Dinechin @ 2022-04-28 11:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Murilo Opsfelder Araujo, Jason Wang, virtualization,
linux-kernel, mopsfelder, Christophe de Dinechin
[Resend, still struggling with new laptop email settings]
> On 28 Apr 2022, at 13:03, Michael S. Tsirkin <mst@redhat.com> 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 <cdupontd@redhat.com> wrote:
>>>
>>>
>>>
>>>> On 28 Apr 2022, at 11:46, Christophe Marie Francois Dupont de Dinechin <cdupontd@redhat.com> wrote:
>>>>
>>>>
>>>>
>>>>> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <muriloo@linux.ibm.com> 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 <muriloo@linux.ibm.com>
>>>>>> ---
>>>>>> 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/20220414150855.2407137-4-dinechin@redhat.com/
>>>>>
>>>>> 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 <dinechin@redhat.com>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs()
@ 2022-04-28 11:52 ` Christophe Marie Francois Dupont de Dinechin
0 siblings, 0 replies; 13+ messages in thread
From: Christophe Marie Francois Dupont de Dinechin @ 2022-04-28 11:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, virtualization, mopsfelder,
Murilo Opsfelder Araujo, Christophe de Dinechin
[Resend, still struggling with new laptop email settings]
> On 28 Apr 2022, at 13:03, Michael S. Tsirkin <mst@redhat.com> 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 <cdupontd@redhat.com> wrote:
>>>
>>>
>>>
>>>> On 28 Apr 2022, at 11:46, Christophe Marie Francois Dupont de Dinechin <cdupontd@redhat.com> wrote:
>>>>
>>>>
>>>>
>>>>> On 15 Apr 2022, at 05:51, Murilo Opsfelder Araújo <muriloo@linux.ibm.com> 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 <muriloo@linux.ibm.com>
>>>>>> ---
>>>>>> 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/20220414150855.2407137-4-dinechin@redhat.com/
>>>>>
>>>>> 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 <dinechin@redhat.com>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-04-28 11:53 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-15 2:30 [PATCH] virtio-pci: Remove wrong address verification in vp_del_vqs() Murilo Opsfelder Araujo
2022-04-15 3:51 ` Murilo Opsfelder Araújo
2022-04-28 9:46 ` Christophe Marie Francois Dupont de Dinechin
2022-04-28 9:46 ` Christophe Marie Francois Dupont de Dinechin
2022-04-28 9:51 ` Christophe Marie Francois Dupont de Dinechin
2022-04-28 9:51 ` Christophe Marie Francois Dupont de Dinechin
2022-04-28 9:55 ` Christophe Marie Francois Dupont de Dinechin
2022-04-28 9:55 ` Christophe Marie Francois Dupont de Dinechin
2022-04-28 11:03 ` Michael S. Tsirkin
2022-04-28 11:03 ` Michael S. Tsirkin
2022-04-28 11:43 ` Christophe Marie Francois Dupont de Dinechin
2022-04-28 11:52 ` Christophe Marie Francois Dupont de Dinechin
2022-04-28 11:52 ` Christophe Marie Francois Dupont de Dinechin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.