linux-kernel - Re: [PATCH] ALSA: snd-usb: fix IRQ triggered NULL pointer dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87inq1i13l.fsf@adiPC.i-did-not-set--mail-host-address--so-tickle-me>
Date:   Fri, 30 Dec 2016 23:00:30 +0200
From:   Ioan-Adrian Ratiu <adi@...rat.com>
To:     Takashi Iwai <tiwai@...e.de>
Cc:     Takashi Sakamoto <o-takashi@...amocchi.jp>, clemens@...isch.de,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ALSA: snd-usb: fix IRQ triggered NULL pointer dereference

On Fri, 30 Dec 2016, Takashi Iwai <tiwai@...e.de> wrote:
> On Wed, 21 Dec 2016 11:38:54 +0100,
> Ioan-Adrian Ratiu wrote:
>> 
>> >> > Please take the time to fully analyze my patch and let's have a
>> >> > discussion based on it, not reject it outright and replace it with
>> >> > a quick and dirty delay hack.
>> >> 
>> >> OK. I'll deliberately check it again so that I have no overlooks. I
>> >> with this thread also catch the other developers enough helpful to
>> >> you. (I just eventually caught your patch in LKML and not developer
>> >> for this category of devices.)
>> >
>> > Sorry for the late reply, as I've been (still) off and had bad net
>> > connections.
>> >
>> > About your fix: Sakamoto-san is right, it's no good way to fix this
>> > kind or problem.  The easiest option right now is just to revert my
>> > previous fix, as it obviously introduces another regression.  The
>> > correct fix will be taken after that.
>> >
>> > I'm going to prepare a revert patch and ask Linus to take it before
>> > rc1.
>> 
>> I agree with reverting the initial commit decision because my problem
>> disappears with it.
>> 
>> But can you please state a reason for why my patch is "no good way to
>> fix"? Being too intrusive is not a good reason.
>
> "Being too intrusive" is the exact reason why it's not good as a
> "regression fix" like this case.  The logic you've implemented in the
> patch itself looks good (although the code introduces a bug, the
> unbalance of snd_usb_*lock_shutdown()).  The only point I couldn't
> take it is that it's rather a fundamental change, not a quick fix for
> a regression.
>
> What's the worst case scenario in a regression fix?  It's when a fix
> introduces yet another regression.  It'd be much worse if the new
> regression is deeper.  The complicated logical change has a potential
> risk of such.  Thus an intrusive change is not always good as a
> "regression fix".
>
> In general, if the change were trivial, it's obviously OK to take as a
> regression fix -- where the logic is also trivial in most cases, too.
> But when the fix becomes complex, we need to rethink.  Especially when
> the original buggy commit is small, reverting it is often better as a
> clear cut.
>
> Think in that way: you're addressing a deeper problem that was
> revealed accidentally by my previous bad fix.  Writing the change as
> if it were merely a regression fix gives the wrong understanding to
> readers :)

Yes, this makes sense. Thank you.

>
> That said, I'd appreciate if you respin the fix again with a
> combination of my previous fix and brush up the code a bit as a whole,
> and document it not as a regression fix but as a complete fix of the
> existing races.  I can write it in my side quickly, but it'd be almost
> in the same form as you posted, I suppose.

I'll find some time to do a rebase either tomorrow (31 Dec) or until
2 Jan at the latest.

Ionel

>
>
> thanks,
>
> Takashi