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 Fri, 11 Mar 2022 11:00:24 +0800
Stephen Hemminger <[email protected]> wrote:
>> + if (!pcpu_sum) {
>> + for (j = 0; j < i; j++)
>> + data[j] = 0;
>> + return
>
> Why is unrolled zero (memset) needed? The data area comes from
> ethtool_get_stats and is already zeroed (vzalloc).
>
>
> There does look like at TOCTOU error here with on the number of stats.
> Code doesn't look hotplug safe.
> Not sure, but that issue might have been raised during review.
I unrolled the 'data area' since the three 'for loops' before
have already assigned the value to the data area.
And I have not found any review about it.
Thanks,
Jiang
On Fri, 11 Mar 2022 10:43:44 +0800
Jiasheng Jiang <[email protected]> wrote:
> + if (!pcpu_sum) {
> + for (j = 0; j < i; j++)
> + data[j] = 0;
> + return
Why is unrolled zero (memset) needed? The data area comes from
ethtool_get_stats and is already zeroed (vzalloc).
There does look like at TOCTOU error here with on the number of stats.
Code doesn't look hotplug safe.
Not sure, but that issue might have been raised during review.
On Fri, Mar 11, 2022 at 11:20:35AM +0800, Jiasheng Jiang 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,
greg k-h
On Fri, 11 Mar 2022 11:20:35 +0800
Jiasheng Jiang <[email protected]> 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;
> + }
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);