linux-kernel - Re: [GIT PULL] Driver core fixes for 5.7-rc7

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 May 2020 09:33:10 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Sasha Levin <sashal@...nel.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Stephen Rothwell <sfr@...b.auug.org.au>
Subject: Re: [GIT PULL] Driver core fixes for 5.7-rc7 - take 2

On Sun, May 24, 2020 at 11:42:19AM -0400, Sasha Levin wrote:
> On Sun, May 24, 2020 at 05:00:18PM +0200, Greg KH wrote:
> > On Sat, May 23, 2020 at 11:14:28AM -0700, Linus Torvalds wrote:
> > > On Sat, May 23, 2020 at 8:29 AM Greg KH <gregkh@...uxfoundation.org> wrote:
> > > >
> > > > The kobject patch that was originally in here has now been reverted, as
> > > > Guenter reported boot problems with it on some of his systems.
> > > 
> > > Hmm. That original patch looks obviously buggy: in kobject_cleanup()
> > > it would end up doing "kobject_put(parent)" regardless of whether it
> > > had actually done __kobject_del() or not.
> > > 
> > > That _could_ have been intentional, but considering the commit
> > > message, it clearly wasn't in this case.  It might be worth re-trying
> > > to the commit, just with that fixed.
> > 
> > Turns out that wasn't the real problem here, the culprit is the
> > lib/test_printf.c code trying to tear down a kobject tree from the
> > parent down to the children (i.e. in the backwards order).
> > 
> > > Btw, when you end up reverting a patch that was already the top patch,
> > > you might as well just remove it entirely from that tree instead (ie
> > > "git reset --hard HEAD^" instead of "git revert HEAD").
> > > 
> > > Unless somebody else uses your branches and you are afraid that the
> > > non-reverted commit escaped out in the wild that way?
> > 
> > I don't like rebasing or changing the HEAD like that on a public branch.
> > As proof, syzbot started sending me a bunch of "this is the failed
> > commit" messages right after your email, based on it's testing of the
> > tree in linux-next.
> 
> OTOH, leaving commits like this may result in confusion later on because
> of confusion around the "correct" patch.
> 
> Consider this:
> 
> 1. Someone writes a patch named "close memory leak when freeing XYZ"
> 2. We revert it a day later with 'Revert "close memory leak when
> freeing XYZ"'

And the sha1 is in the commit, showing which patch was reverted.

> 3. Now, what would the author of the original patch do? That's right -
> re-submit a patch with an identical subject line and patch description,
> but with a subtle change in the code to fix the bug the original patch
> was reverted for.

Sometimes, yes, but sometimes, as in this case, a totally different
patch will be submitted for the problem :)

But, even if it was there, the sha1 in the revert should allow us to
track this properly.  I can't remember a time where this has caused
problems in the past, can you?

> So now we end up with two "close memory leak when freeing XYZ" commits
> in our git history that are nearly identical. Recipe for a disaster :)

Time and sha1 should show them being different :)

thanks,

greg k-h