Fix possible crash when resctrl gets remounted

[libvirt] [PATCH v2 1/2] util: Check if kernel-provided info is consistent with itself

Posted by Martin Kletzander 7 years, 3 months ago

Just in case someone re-mounted /sys/fs/resctrl with different mount
options (cdp), add a check here.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1540780

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/util/virresctrl.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index ef388757a704..6860e86e649d 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -941,6 +941,17 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl,
     if (!mask)
         return -1;
 
+    if (!resctrl ||
+        level >= resctrl->nlevels ||
+        !resctrl->levels[level] ||
+        !resctrl->levels[level]->types[type]) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Missing or inconsistent resctrl info for "
+                         "level '%ud' type '%s'"),
+                       level, virCacheTypeToString(type));
+        goto cleanup;
+    }
+
     if (virBitmapShrink(mask, resctrl->levels[level]->types[type]->bits) < 0)
         goto cleanup;
 
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 1/2] util: Check if kernel-provided info is consistent with itself

Posted by Peter Krempa 7 years, 3 months ago

On Fri, Feb 02, 2018 at 15:27:21 +0100, Martin Kletzander wrote:
> Just in case someone re-mounted /sys/fs/resctrl with different mount
> options (cdp), add a check here.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1540780
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  src/util/virresctrl.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index ef388757a704..6860e86e649d 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -941,6 +941,17 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl,
>      if (!mask)
>          return -1;
>  
> +    if (!resctrl ||
> +        level >= resctrl->nlevels ||
> +        !resctrl->levels[level] ||
> +        !resctrl->levels[level]->types[type]) {

The only caller of this function checks the range of type by converting
it from string with 'virResctrlTypeFromString' but the type in this
function is 'virCacheType' and you use 'virCacheTypeToString'.

Given the inconsistency and the fact that C will not validate the passed
type in this case you should also validate that 'type' has the correct
range.

ACK with that check added.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 1/2] util: Check if kernel-provided info is consistent with itself

Posted by Martin Kletzander 7 years, 3 months ago

On Fri, Feb 02, 2018 at 06:02:22PM +0100, Peter Krempa wrote:
>On Fri, Feb 02, 2018 at 15:27:21 +0100, Martin Kletzander wrote:
>> Just in case someone re-mounted /sys/fs/resctrl with different mount
>> options (cdp), add a check here.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1540780
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  src/util/virresctrl.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>> index ef388757a704..6860e86e649d 100644
>> --- a/src/util/virresctrl.c
>> +++ b/src/util/virresctrl.c
>> @@ -941,6 +941,17 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl,
>>      if (!mask)
>>          return -1;
>>
>> +    if (!resctrl ||
>> +        level >= resctrl->nlevels ||
>> +        !resctrl->levels[level] ||
>> +        !resctrl->levels[level]->types[type]) {
>
>The only caller of this function checks the range of type by converting
>it from string with 'virResctrlTypeFromString' but the type in this
>function is 'virCacheType' and you use 'virCacheTypeToString'.
>
>Given the inconsistency and the fact that C will not validate the passed
>type in this case you should also validate that 'type' has the correct
>range.
>
>ACK with that check added.

There are three "inconsistent" types, but they all use VIR_CACHE_TYPE_LAST in
their VIR_ENUM_IMPL:

- virResctrl - That's the naming we need to use when writing into resctrl schemata file

- virCache - That's the naming that was decide upstream that we should use in our XML

- virCacheKernel - That's what kernel uses when we read from cache info from sysfs

I have no problem with adding one extra check here, but I think this is taken
care of by the fact that they all use the same enum for the implementation.  I
can just extract the const char * fromt he error message before the check and
just add `str != NULL` in the condition.  I'll leave that choice up to you.  I
agree the code is not as nice as it could be and I'm already working on at least
the minimal refactoring that needs tobe done, but in the meantime I'd like to
have this working at least.  The number of workarounds we need to make due to
IMNSHO poor implementation of resctrl in kernel is sickening^Wtiring let's say.

Just so we don't misunderstand each other, I'm not against any changes to that
code.  Feel free to suggest any other things that might be made better, I'm all
for making this nicer.  It's just that most of the time I couldn't find any
nicer way to do some of these _suboptimal_ things.

Have a nice day,
Martin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 1/2] util: Check if kernel-provided info is consistent with itself

Posted by Peter Krempa 7 years, 3 months ago

On Mon, Feb 05, 2018 at 10:37:25 +0100, Martin Kletzander wrote:
> On Fri, Feb 02, 2018 at 06:02:22PM +0100, Peter Krempa wrote:
> > On Fri, Feb 02, 2018 at 15:27:21 +0100, Martin Kletzander wrote:
> > > Just in case someone re-mounted /sys/fs/resctrl with different mount
> > > options (cdp), add a check here.
> > > 
> > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1540780
> > > 
> > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> > > ---
> > >  src/util/virresctrl.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> > > index ef388757a704..6860e86e649d 100644
> > > --- a/src/util/virresctrl.c
> > > +++ b/src/util/virresctrl.c
> > > @@ -941,6 +941,17 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl,
> > >      if (!mask)
> > >          return -1;
> > > 
> > > +    if (!resctrl ||
> > > +        level >= resctrl->nlevels ||
> > > +        !resctrl->levels[level] ||
> > > +        !resctrl->levels[level]->types[type]) {
> > 
> > The only caller of this function checks the range of type by converting
> > it from string with 'virResctrlTypeFromString' but the type in this
> > function is 'virCacheType' and you use 'virCacheTypeToString'.
> > 
> > Given the inconsistency and the fact that C will not validate the passed
> > type in this case you should also validate that 'type' has the correct
> > range.
> > 
> > ACK with that check added.
> 
> There are three "inconsistent" types, but they all use VIR_CACHE_TYPE_LAST in
> their VIR_ENUM_IMPL:
> 
> - virResctrl - That's the naming we need to use when writing into resctrl schemata file
> 
> - virCache - That's the naming that was decide upstream that we should use in our XML
> 
> - virCacheKernel - That's what kernel uses when we read from cache info from sysfs
> 
> I have no problem with adding one extra check here, but I think this is taken
> care of by the fact that they all use the same enum for the implementation.  I
> can just extract the const char * fromt he error message before the check and
> just add `str != NULL` in the condition.  I'll leave that choice up to you.  I
> agree the code is not as nice as it could be and I'm already working on at least
> the minimal refactoring that needs tobe done, but in the meantime I'd like to
> have this working at least.  The number of workarounds we need to make due to
> IMNSHO poor implementation of resctrl in kernel is sickening^Wtiring let's say.
> 
> Just so we don't misunderstand each other, I'm not against any changes to that
> code.  Feel free to suggest any other things that might be made better, I'm all
> for making this nicer.  It's just that most of the time I couldn't find any
> nicer way to do some of these _suboptimal_ things.

Hmm, okay fair enough. Maybe just add a comment to the enum
implementations that they should all be terminated via VIR_CACHE_TYPE_LAST
since code depends on it.

It can definitely be separate in this case.

ACK to this patch as-is.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list