Re: [PATCH v2] x86/resctrl: Avoid overflow in MB settings in bw_validate()

[Reinette Chatre]

Hi Martin,

On 9/17/24 1:12 AM, Martin Kletzander wrote:
> On Mon, Sep 16, 2024 at 09:56:01AM -0700, Reinette Chatre wrote:
> > On 9/16/24 6:07 AM, Martin Kletzander wrote:
> > > When resctrl is mounted with the "mba_MBps" option the default (maximum)
> > > bandwidth is the maximum unsigned value for the type.  However when
> > > using the same value that already exists in the schemata file it is then
> > > rounded up to the bandwidth granularity and overflows to a small number
> > > instead, making it difficult to reset memory bandwidth allocation value
> > > back to its default.
> > >
> > > Since the granularity and minimum bandwidth are not used when the
> > > software controller is used (resctrl is mounted with the "mba_MBps"),
> > > skip the rounding up as well and return early from bw_validate().
> >
> > Thank you very much for finding the issue and proposing a fix.
> >
> > > Signed-off-by: Martin Kletzander <nert.pinx@xxxxxxxxx>
> > > ---
> > > Changes in v2:
> > >   - actually save the value in the output parameter @data
> > >
> > >   arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 9 +++++++--
> > >   1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > > index 50fa1fe9a073..702b1a372e9c 100644
> > > --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > > +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > > @@ -48,8 +48,13 @@ static bool bw_validate(char *buf, unsigned long *data, struct rdt_resource *r)
> > >           return false;
> > >       }
> > >
> > > -    if ((bw < r->membw.min_bw || bw > r->default_ctrl) &&
> > > -        !is_mba_sc(r)) {
> > > +    /* Nothing else to do if software controller is enabled */
> > > +    if (is_mba_sc(r)) {
> > > +        *data = bw;
> > > +        return true;
> >
> > While this would fix the scenario tested I do not believe this fully addresses the
> > overflow issue. As I understand the test wrote U32_MAX to the schemata file,
> > which triggered the overflow because of the rounding and is fixed by this patch. Looks like,
> > after this patch, writing "U32_MAX + 1" will still trigger the overflow.
> >
> > The overflow appears to result from some inconsistent type use and not using
> > appropriate parsing API that is able to detect overflow.
> >
> > How about something like below:
>
> That makes much more sense, I have not considered changing the data type
> as I wanted to keep the changes at minimum, but your solution is even
> better.  Should I leave the fix up to you or do you want me to send a v3?

Could you please try it out to ensure it works for you and then send a v3?

Thank you very much.

Reinette