Received-SPF: pass (google.com: domain of linux-kernel+bounces-151850-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240118181954.1415197-1-zokeefe@google.com> <20240417111001.fa2eg5gp6t2wiwco@quack3>
 <CAAa6QmSOum_0ZhyUq1ppguLp0jpEs0u1U843GkF==xMwaMGV4A@mail.gmail.com> <20240418110434.g5bx5ntp2m4433eo@quack3>
In-Reply-To: <20240418110434.g5bx5ntp2m4433eo@quack3>
From: "Zach O'Keefe" <zokeefe@google.com>
Date: Fri, 19 Apr 2024 11:04:31 -0700
Message-ID: <CAAa6QmS-PwoR7QGr5f-yUrMsnszb=q8+wpjKrYzsv0OsgdP4jw@mail.gmail.com>
Subject: Re: [PATCH] mm/writeback: fix possible divide-by-zero in
 wb_dirty_limits(), again
To: Jan Kara <jack@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org, Maxim Patlasov <MPatlasov@parallels.com>, 
	stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 18, 2024 at 4:04=E2=80=AFAM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 17-04-24 12:33:39, Zach O'Keefe wrote:
> > On Wed, Apr 17, 2024 at 4:10=E2=80=AFAM Jan Kara <jack@suse.cz> wrote:
> > > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > > > index cd4e4ae77c40a..02147b61712bc 100644
> > > > --- a/mm/page-writeback.c
> > > > +++ b/mm/page-writeback.c
> > > > @@ -1638,7 +1638,7 @@ static inline void wb_dirty_limits(struct dir=
ty_throttle_control *dtc)
> > > >        */
> > > >       dtc->wb_thresh =3D __wb_calc_thresh(dtc);
> > > >       dtc->wb_bg_thresh =3D dtc->thresh ?
> > > > -             div_u64((u64)dtc->wb_thresh * dtc->bg_thresh, dtc->th=
resh) : 0;
> > > > +             div64_u64(dtc->wb_thresh * dtc->bg_thresh, dtc->thres=
h) : 0;
> ...
> > > Thirdly, if thresholds are larger than 1<<32 pages, then dirty balanc=
ing is
> > > going to blow up in many other spectacular ways - consider only the
> > > multiplication on this line - it will not necessarily fit into u64 an=
ymore.
> > > The whole dirty limiting code is interspersed with assumptions that l=
imits
> > > are actually within u32 and we do our calculations in unsigned longs =
to
> > > avoid worrying about overflows (with occasional typing to u64 to make=
 it
> > > more interesting because people expected those entities to overflow 3=
2 bits
> > > even on 32-bit archs). Which is lame I agree but so far people don't =
seem
> > > to be setting limits to 16TB or more. And I'm not really worried abou=
t
> > > security here since this is global-root-only tunable and that has muc=
h
> > > better ways to DoS the system.
> > >
> > > So overall I'm all for cleaning up this code but in a sensible way pl=
ease.
> > > E.g. for these overflow issues at least do it one function at a time =
so
> > > that we can sensibly review it.
> > >
> > > Andrew, can you please revert this patch until we have a better fix? =
So far
> > > it does more harm than good... Thanks!
> >
> > Shall we just roll-forward with a suitable fix? I think all the
> > original code actually "needed" was to cast the ternary predicate,
> > like:
> >
> > ---8<---
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index fba324e1a010..ca1bfc0c9bdd 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -1637,8 +1637,8 @@ static inline void wb_dirty_limits(struct
> > dirty_throttle_control *dtc)
> >          *   at some rate <=3D (write_bw / 2) for bringing down wb_dirt=
y.
> >          */
> >         dtc->wb_thresh =3D __wb_calc_thresh(dtc);
> > -       dtc->wb_bg_thresh =3D dtc->thresh ?
> > -               div64_u64(dtc->wb_thresh * dtc->bg_thresh, dtc->thresh)=
 : 0;
> > +       dtc->wb_bg_thresh =3D (u32)dtc->thresh ?
> > +               div_u64((u64)dtc->wb_thresh * dtc->bg_thresh, dtc->thre=
sh) : 0;
>
> Well, this would fix the division by 0 but when you read the code you
> really start wondering what's going on :) [..]

Ya, this was definitely a local fix in an area of code I know very
little abit. I stumbled across it in a rather contrived way -- made
easier by internal patches -- and felt its existence still warranted a
local fix.

> [..] And as I wrote above when
> thresholds pass UINT_MAX, the dirty limitting code breaks down anyway so =
I
> don't think the machine will be more usable after your fix. Would you be =
up
> for a challenge to modify mm/page-writeback.c so that such huge limits
> cannot be set instead? That would be actually a useful fix...

:) I can't say my schedule affords me much time to take on any
significant unplanned work. Perhaps as a Friday afternoon exercise
I'll come back to scope this out, driven by some sense of
responsibility garnered from starting down this path ; but ... my TODO
list is long.

Have a great rest of your day / weekend,
Zach

>                                                                 Honza
>
> >
> >         /*
> >          * In order to avoid the stacked BDI deadlock we need
> > ---8<---
> >
> > Thanks, and apologize for the inconvenience
> >
> > Zach
> >
> > >                                                                 Honza
> > > --
> > > Jan Kara <jack@suse.com>
> > > SUSE Labs, CR
> >
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR