As the potential failure of the kvmalloc_array(),
it should be better to check and restore the 'data'
if fails in order to avoid the dereference of the
NULL pointer.
Fixes: 6ae746711263 ("hv_netvsc: Add per-cpu ethtool stats for netvsc")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
drivers/net/hyperv/netvsc_drv.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 3646469433b1..018c4a5f6f44 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1587,6 +1587,12 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
pcpu_sum = kvmalloc_array(num_possible_cpus(),
sizeof(struct netvsc_ethtool_pcpu_stats),
GFP_KERNEL);
+ if (!pcpu_sum) {
+ for (j = 0; j < i; j++)
+ data[j] = 0;
+ return;
+ }
+
netvsc_get_pcpu_stats(dev, pcpu_sum);
for_each_present_cpu(cpu) {
struct netvsc_ethtool_pcpu_stats *this_sum = &pcpu_sum[cpu];
--
2.25.1
On Sat, 12 Mar 2022 01:25:38 +0800
Stephen Hemminger <[email protected]> wrote:
>> + if (!pcpu_sum) {
>> + for (j = 0; j < i; j++)
>> + data[j] = 0;
>> + return;
>> + }
>
> I don't think you understood what my comment was.
>
> The zeroing here is not necessary. Just do:
> if (!pcpu_sum)
> return;
>
> The data pointer is to buffer allocated here:
>
> static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
> {
> ...
> if (n_stats) {
> data = vzalloc(array_size(n_stats, sizeof(u64))); <<<<< is already zeroed.
> if (!data)
> return -ENOMEM;
> ops->get_ethtool_stats(dev, &stats, data);
OK, I will submit a v2 to remove them.
Jiang
On Mon, Mar 14, 2022 at 04:05:14PM +0800, Jiasheng Jiang wrote:
> On Mon, Mar 14, 2022 at 03:33:49PM +0800, Greg KH wrote:
> >> Thanks, I have tested the patch by kernel_patch_verify,
> >
> > What is that?
>
> It a Linux kernel patch static verification helper tool.
> Link: https://github.com/nmenon/kernel_patch_verify
>
> >> and all the tests are passed.
> >
> > What tests exactly? How did you fail this allocation?
>
> The failure of allocation is not included in the tests.
> And as far as I know, there is not any tool that has the
> ability to fail the allocation.
There are tools that do this.
> But I think that for safety, the cost of redundant and harmless
> check is acceptable.
> Also, checking after allocation is a good program pattern.
That's fine, it's how you clean up that is the problem that not everyone
gets correct, which is why it is good to verify that you do not
introduce problems.
greg k-h
On Mon, Mar 14, 2022 at 03:33:49PM +0800, Greg KH wrote:
>> Thanks, I have tested the patch by kernel_patch_verify,
>
> What is that?
It a Linux kernel patch static verification helper tool.
Link: https://github.com/nmenon/kernel_patch_verify
>> and all the tests are passed.
>
> What tests exactly? How did you fail this allocation?
The failure of allocation is not included in the tests.
And as far as I know, there is not any tool that has the
ability to fail the allocation.
But I think that for safety, the cost of redundant and harmless
check is acceptable.
Also, checking after allocation is a good program pattern.
Thanks,
Jiang
On Fri, Mar 11, 2022 at 02:43:48PM +0800, Greg KH wrote:
>> As the potential failure of the kvmalloc_array(),
>> it should be better to check and restore the 'data'
>> if fails in order to avoid the dereference of the
>> NULL pointer.
>>
>> Fixes: 6ae746711263 ("hv_netvsc: Add per-cpu ethtool stats for netvsc")
>> Signed-off-by: Jiasheng Jiang <[email protected]>
>> ---
>> drivers/net/hyperv/netvsc_drv.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
>> index 3646469433b1..018c4a5f6f44 100644
>> --- a/drivers/net/hyperv/netvsc_drv.c
>> +++ b/drivers/net/hyperv/netvsc_drv.c
>> @@ -1587,6 +1587,12 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
>> pcpu_sum = kvmalloc_array(num_possible_cpus(),
>> sizeof(struct netvsc_ethtool_pcpu_stats),
>> GFP_KERNEL);
>> + if (!pcpu_sum) {
>> + for (j = 0; j < i; j++)
>> + data[j] = 0;
>> + return;
>> + }
>
>How did you test this to verify it is correct?
Thanks, I have tested the patch by kernel_patch_verify,
and all the tests are passed.
Jiang
On Mon, 14 Mar 2022 16:35:00 +0800 Jiasheng Jiang wrote:
> On Mon, Mar 14, 2022 at 04:13:59PM +0800, Greg KH wrote:
> >> The failure of allocation is not included in the tests.
> >> And as far as I know, there is not any tool that has the
> >> ability to fail the allocation.
> >
> > There are tools that do this.
> >
>
> Thanks, could you please tell me the tools?
Google "linux kernel fail allocation test"
second result is "Fault injection capabilities infrastructure"
which is what you're looking for.
Please try harder next time.
On Mon, Mar 14, 2022 at 03:33:49PM +0800, Jiasheng Jiang wrote:
> On Fri, Mar 11, 2022 at 02:43:48PM +0800, Greg KH wrote:
> >> As the potential failure of the kvmalloc_array(),
> >> it should be better to check and restore the 'data'
> >> if fails in order to avoid the dereference of the
> >> NULL pointer.
> >>
> >> Fixes: 6ae746711263 ("hv_netvsc: Add per-cpu ethtool stats for netvsc")
> >> Signed-off-by: Jiasheng Jiang <[email protected]>
> >> ---
> >> drivers/net/hyperv/netvsc_drv.c | 6 ++++++
> >> 1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> >> index 3646469433b1..018c4a5f6f44 100644
> >> --- a/drivers/net/hyperv/netvsc_drv.c
> >> +++ b/drivers/net/hyperv/netvsc_drv.c
> >> @@ -1587,6 +1587,12 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
> >> pcpu_sum = kvmalloc_array(num_possible_cpus(),
> >> sizeof(struct netvsc_ethtool_pcpu_stats),
> >> GFP_KERNEL);
> >> + if (!pcpu_sum) {
> >> + for (j = 0; j < i; j++)
> >> + data[j] = 0;
> >> + return;
> >> + }
> >
> >How did you test this to verify it is correct?
>
> Thanks, I have tested the patch by kernel_patch_verify,
What is that?
> and all the tests are passed.
What tests exactly? How did you fail this allocation?
thanks,
greg k-h
On Mon, Mar 14, 2022 at 04:13:59PM +0800, Greg KH wrote:
>> The failure of allocation is not included in the tests.
>> And as far as I know, there is not any tool that has the
>> ability to fail the allocation.
>
> There are tools that do this.
>
Thanks, could you please tell me the tools?
Jiang
>> But I think that for safety, the cost of redundant and harmless
>> check is acceptable.
>> Also, checking after allocation is a good program pattern.
>
> That's fine, it's how you clean up that is the problem that not everyone
> gets correct, which is why it is good to verify that you do not
> introduce problems.