Re: [MAINTAINER SUMMIT] Rethinking the acceptance policy for "trivial" patches

[Leon Romanovsky <leon@kernel.org>]

On Fri, Apr 23, 2021 at 11:06:43AM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 22 Apr 2021 09:38:03 -0600
> Shuah Khan <skhan@linuxfoundation.org> escreveu:
> 
> > On 4/22/21 6:18 AM, Leon Romanovsky wrote:
> > > On Thu, Apr 22, 2021 at 11:55:11AM +0200, Mauro Carvalho Chehab wrote:  
> > >> Em Thu, 22 Apr 2021 09:34:38 +0200
> > >> Geert Uytterhoeven <geert@linux-m68k.org> escreveu:
> > >>  
> > >>> On Wed, Apr 21, 2021 at 11:50 PM James Morris <jmorris@namei.org> wrote:  
> > >>>> On Wed, 21 Apr 2021, Julia Lawall wrote:  
> > >>>>> The apology states that they didn't detect any vulnerabilities.  They
> > >>>>> found three non exploitable bugs and submitted incorrect patches for them.
> > >>>>> When the patches received some positive feedback, they explained that the
> > >>>>> patches were incorrect and provided a proper fix.
> > >>>>>
> > >>>>> So they damaged trust, but not actually the Linux kernel...  
> > >>>>
> > >>>> The issue is that there was no consent and no coordination, so we don't
> > >>>> know the scope of the experiment and whether it was still continuing.
> > >>>>
> > >>>> We are this not able to trust anything the group said about what they'd
> > >>>> done or not done, until now [1].
> > >>>>
> > >>>> In all probability there is nothing further amiss but we would not have
> > >>>> expected them to use fake gmail accounts to submit bugs to the kernel
> > >>>> either.
> > >>>>
> > >>>> It's now on us to audit all of their known contributions, because we don't
> > >>>> know the scope of the experiment, which was based on the use of deception,
> > >>>> and we can't make any assumptions based on what they have said.
> > >>>>
> > >>>> We also need the identity of the 'random' gmail accounts they used,
> > >>>> although this should be handled by a small trusted group in private, as it
> > >>>> will lead to privacy issues for kernel maintainers who responded to them
> > >>>> in public.  
> > >>>
> > >>> What do we gain by blindly reverting all[1] umn.edu patches, and
> > >>> ignoring future patches from umn.edu?
> > >>> I think all of this is moot: other people may be doing the same thing,
> > >>> or even "in worse faith".  The only thing that helps is making sure
> > >>> patches get reviewed[2] before being applied.
> > >>>
> > >>> [1] Judging from the new review comments, many of the 190 patches
> > >>>      to be reverted were real fixes.  
> > >>
> > >> The reverted ones for media (29 patches) didn't contain any malicious code.
> > >> One was useless (because the media core already fixes the pointed issue),
> > >> but the other ones were valid patches.  
> > > 
> > > I'm sorry that I didn't check all media commits, but this random commit
> > > 467a37fba93f ("media: dvb: Add check on sp8870_readreg") has a bug and
> > > broke sp8870 (I don't know what is it).
> > > 
> > > diff --git a/drivers/media/dvb-frontends/sp8870.c b/drivers/media/dvb-frontends/sp8870.c
> > > index 8d31cf3f4f07..270a3c559e08 100644
> > > --- a/drivers/media/dvb-frontends/sp8870.c
> > > +++ b/drivers/media/dvb-frontends/sp8870.c
> > > @@ -293,7 +293,9 @@ static int sp8870_set_frontend_parameters(struct dvb_frontend *fe)
> > >          sp8870_writereg(state, 0xc05, reg0xc05);
> > > 
> > >          // read status reg in order to clear pending irqs
> > > -       sp8870_readreg(state, 0x200);
> > > +       err = sp8870_readreg(state, 0x200);
> > > +       if (err)
> > > +               return err;
> > > 
> > >          // system controller start
> > >          sp8870_microcontroller_start(state);
> > > 
> > > 
> > >     67 static int sp8870_readreg (struct sp8870_state* state, u16 reg)
> > >     68 {
> > >     69         int ret;
> > >   <...>
> > >     77         if (ret != 2) {
> > >     78                 dprintk("%s: readreg error (ret == %i)\n", __func__, ret);
> > >     79                 return -1;
> > >     80         }
> > >     81
> > >     82         return (b1[0] << 8 | b1[1]);
> > >     83 }
> > > 
> > > The valid check should be if (err < 0);
> > >   
> > 
> > Correct. Like all the other callers of sp8870_readreg() do with
> > its return. Non-zero return is valid for this routine.
> 
> This particular patch is completely broken and should be reverted.

This is exactly the point, many patches from @umn are broken and the
right way to check them is to checkout to the time when they were
introduced.

I just wanted to show you that you claim about validity of patches is
not accurate.

"The reverted ones for media (29 patches) didn't contain any malicious code.
 One was useless (because the media core already fixes the pointed issue),
 out the other ones were valid patches."

Thanks