Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 11 Mar 2021 10:44:28 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Li Li <dualli@chromium.org>
Cc:     devel@driverdev.osuosl.org, kernel-team@android.com,
        linux-kernel@vger.kernel.org, joel@joelfernandes.org,
        arve@android.com, Martijn Coenen <maco@google.com>,
        hridya@google.com, surenb@google.com, christian@brauner.io,
        Todd Kjos <tkjos@google.com>
Subject: Re: [PATCH v1 1/3] binder: BINDER_FREEZE ioctl
Message-ID: <YEnmfM+VmVPUIZ4W@kroah.com>
References: <20210310225251.2577580-1-dualli@chromium.org>
 <20210310225251.2577580-2-dualli@chromium.org>
 <YEnH1qd527g/neY4@kroah.com>
 <CA+xfxX4tD30BJLgwLAoiMzF7xTC-4q1i0A5Znp3tJzyi3ATLzQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CA+xfxX4tD30BJLgwLAoiMzF7xTC-4q1i0A5Znp3tJzyi3ATLzQ@mail.gmail.com>
Precedence: bulk

On Thu, Mar 11, 2021 at 01:36:26AM -0800, Li Li wrote:
> On Wed, Mar 10, 2021 at 11:33 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Mar 10, 2021 at 02:52:49PM -0800, Li Li wrote:
> > >       if (target_proc) {
> > >               binder_inner_proc_lock(target_proc);
> > > +             target_proc->outstanding_txns--;
> > > +             WARN_ON(target_proc->outstanding_txns < 0);
> >
> > WARN_* is a huge crutch, please just handle stuff like this properly and
> > if you really need to, warn userspace (but what can they do about it?)
> >
> > You also just rebooted all systems that have panic-on-warn set, so if
> > this can be triggered by userspace, you caused a DoS of things :(
> >
> > So please remove all of the WARN_ON() you add in this patch series to
> > properly handle the error conditions and deal with them correctly.
> >
> > And if these were here just for debugging, hopefully the code works
> > properly now and you do not need debugging anymore so they can all just
> > be dropped.
> 
> When the target_proc is freed, there's no outstanding transactions already.
> The FREEZE ioctl from userspace won't trigger this. It's for debugging.

It's ok to test for this, to verify all is good, just do not reboot
people's machines if the test fails :)

thanks,

greg k-h