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

[Shuah Khan <skhan@linuxfoundation.org>]

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.

thanks,
-- Shuah