Re: [PATCH 1/1] Revert "iomap: fall back to buffered writes for invalidation failures"

[Lee Jones]

Let's attempt to seek beyond the mud slinging, swearing and the whiny
amateur dramatics for just a brief moment and concentrate solely on
the technicals please.

On Fri, 11 Feb 2022, Matthew Wilcox wrote:
> On Thu, Feb 10, 2022 at 10:15:52AM +0000, Lee Jones wrote:
> > On Wed, 09 Feb 2022, Darrick J. Wong wrote:
> > 
> > > On Wed, Feb 09, 2022 at 08:52:43AM +0000, Lee Jones wrote:
> > > > This reverts commit 60263d5889e6dc5987dc51b801be4955ff2e4aa7.
> > > > 
> > > > Reverting since this commit opens a potential avenue for abuse.
> > > 
> > > What kind of abuse?  Did you conclude there's an avenue solely because
> > > some combination of userspace rigging produced a BUG warning?  Or is
> > > this a real problem that someone found?
> > 
> > Genuine question: Is the ability for userspace to crash the kernel
> > not enough to cause concern?  I would have thought that we'd want to
> > prevent this.
> 
> The kernel doesn't crash.  It's a BUG().  That means it kills the
> task which caused the BUG().  If you've specified that the kernel should
> crash on seeing a BUG(), well, you made that decision, and you get to
> live with the consequences.

BUG() calls are architecture specific.  If no override is provided,
the default appears to panic ("crash") the kernel:

 /*
  * Don't use BUG() or BUG_ON() unless there's really no way out; one
  * example might be detecting data structure corruption in the middle
  * of an operation that can't be backed out of.  If the (sub)system
  * can somehow continue operating, perhaps with reduced functionality,
  * it's probably not BUG-worthy.
  *
  * If you're tempted to BUG(), think again:  is completely giving up
  * really the *only* solution?  There are usually better options, where
  * users don't need to reboot ASAP and can mostly shut down cleanly.
  */
 #ifndef HAVE_ARCH_BUG
 #define BUG() do { \
         printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
         barrier_before_unreachable(); \
         panic("BUG!"); \
 } while (0)
 #endif

The kernel I tested with panics and reboots:

 Kernel panic - not syncing: Fatal exception

Here are the BUG related kernel configs I have set:

 CONFIG_BUG=y                          
 CONFIG_GENERIC_BUG=y                  
 CONFIG_GENERIC_BUG_RELATIVE_POINTERS=y
 # CONFIG_INPUT_EVBUG is not set       
 CONFIG_BUG_ON_DATA_CORRUPTION=y       

Not seeing a "CONFIG_PANIC_ON_BUG" equivalent.  What is missing?

Unless of course you mean disabling BUG support entirely.  In which
case, this is strongly advised against in the help section and I'm not
sure of many development or production kernels that do this.

 config BUG
        bool "BUG() support" if EXPERT
        default y
        help
          Disabling this option eliminates support for BUG and WARN, reducing
          the size of your kernel image and potentially quietly ignoring
          numerous fatal conditions. You should only consider disabling this
          option for embedded systems with no facilities for reporting errors.
          Just say Y.

I've always been under the impression that a BUG() call should never
be triggerable from userspace.  However, I'm always happy to be
incorrect and subsequently reeducated.

In other words ...

Is this a valid issue that you want me to report (in a different way):

> Start again, write a good bug report in a new thread.

Or is this expected behaviour and therefore not a concern:

> > > The BUG report came from page_buffers failing to find any buffer heads
> > > attached to the page.
> > 
> > > Yeah, don't care.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog