Re: [PATCH 5.10 036/103] ucounts: Increase ucounts reference counter before the security hook

[Greg Kroah-Hartman]

On Fri, Sep 03, 2021 at 08:50:39AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 03, 2021 at 07:00:09AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Sep 03, 2021 at 06:57:39AM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Sep 02, 2021 at 01:06:34PM -0500, Eric W. Biederman wrote:
> > > > Sasha Levin <sashal@xxxxxxxxxx> writes:
> > > > 
> > > > > On Wed, Sep 01, 2021 at 12:26:10PM -0500, Eric W. Biederman wrote:
> > > > >>Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
> > > > >>
> > > > >>> On Wed, Sep 01, 2021 at 09:25:25AM -0500, Eric W. Biederman wrote:
> > > > >>>> Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
> > > > >>>>
> > > > >>>> > From: Alexey Gladkov <legion@xxxxxxxxxx>
> > > > >>>> >
> > > > >>>> > [ Upstream commit bbb6d0f3e1feb43d663af089c7dedb23be6a04fb ]
> > > > >>>> >
> > > > >>>> > We need to increment the ucounts reference counter befor security_prepare_creds()
> > > > >>>> > because this function may fail and abort_creds() will try to decrement
> > > > >>>> > this reference.
> > > > >>>>
> > > > >>>> Has the conversion of the rlimits to ucounts been backported?
> > > > >>>>
> > > > >>>> Semantically the code is an improvement but I don't know of any cases
> > > > >>>> where it makes enough of a real-world difference to make it worth
> > > > >>>> backporting the code.
> > > > >>>>
> > > > >>>> Certainly the ucount/rlimit conversions do not meet the historical
> > > > >>>> criteria for backports.  AKA simple obviously correct patches.
> > > > >>>>
> > > > >>>> The fact we have been applying fixes for the entire v5.14 stabilization
> > > > >>>> period is a testament to the code not quite being obviously correct.
> > > > >>>>
> > > > >>>> Without backports the code only affects v5.14 so I have not been
> > > > >>>> including a Cc stable on any of the commits.
> > > > >>>>
> > > > >>>> So color me very puzzled about what is going on here.
> > > > >>>
> > > > >>> Sasha picked this for some reason, but if you think it should be
> > > > >>> dropped, we can easily do so.
> > > > >>
> > > > >>My question is what is the reason Sasha picked this up?
> > > > >>
> > > > >>If this patch even applies to v5.10 the earlier patches have been
> > > > >>backported.  So we can't just drop this patch.  Either the earlier
> > > > >>backports need to be reverted, or we need to make certain all of the
> > > > >>patches are backported.
> > > > >>
> > > > >>I really am trying to understand what is going on and why.
> > > > >
> > > > > I'll happily explain. The commit message is telling us that:
> > > > >
> > > > > 1. There is an issue uncovered by syzbot which this patch fixes:
> > > > >
> > > > > 	"Reported-by: syzbot"
> > > > >
> > > > > 2. The issue was introduced in 905ae01c4ae2 ("Add a reference to ucounts
> > > > > for each cred"):
> > > > >
> > > > > 	"Fixes: 905ae01c4ae2"
> > > > >
> > > > > Since 905ae01c4ae2 exist in 5.10, and this patch seemed to fix an issue,
> > > > > I've queued it up.
> > > > 
> > > > Which begs the question as Alex mentioned how did 905ae01c4ae2 get into
> > > > 5.10, as it was merged to Linus's tree in the merge window for 5.14.
> > > > 
> > > > > In general, if we're missing backports, backported something only
> > > > > partially and should revert it, or anything else that might cause an
> > > > > issue, we'd be more than happy to work with you to fix it up.
> > > > >
> > > > > All the patches we queue up get multiple rounds of emails and reviews,
> > > > > if there is a better way to solicit reviews so that we won't up in a
> > > > > place where you haven't noticed something going in earlier we'd be more
> > > > > than happy to improve that process too.
> > > > 
> > > > I have the bad feeling that 905ae01c4ae2 was backported because it was a
> > > > prerequisite to something with a Fixes tag.
> > > > 
> > > > Fixes tags especially in this instance don't mean code needs to go to
> > > > stable Fixes tags mean that a bug was fixed.  Since I thought the code
> > > > only existed in Linus's tree, I haven't been adding Cc stable or even
> > > > thinking about earlier kernels with respect to this code.
> > > > 
> > > > I honestly can't keep up with the level of review needed for patches
> > > > targeting Linus's tree.  So I occasionally glance at patches destined
> > > > for the stable tree.
> > > > 
> > > > Most of the time it is something being backported without a stable tag,
> > > > but with a fixes tag, that is unnecessary but generally harmless so I
> > > > ignore it.
> > > > 
> > > > In this instance it looks like a whole new feature that has had a rocky
> > > > history and a lot of time to stablize is somehow backported to 5.10 and
> > > > 5.13.  I think all of the known issues are addressed but I won't know
> > > > if all of the issues syzkaller can find are found for another couple of
> > > > weeks.
> > > > 
> > > > Because this code was not obviously correct, because this code did not
> > > > have a stable tag, because I am not even certain it is stable yet,
> > > > I am asking do you know how this code that feels to me like feature work
> > > > wound up being backported?  AKA why is 905ae01c4ae2 in 5.10 and 5.13.
> > > 
> > > Looks like Sasha added it to the tree last week and it went out in the
> > > last set of releases.  Sasha, why was this added?  Let me see if it was
> > > a requirement of some other patch...
> > 
> > Sorry, no, that was this patch, let me get my coffee before I dig into
> > this...
> 
> Ok, it looks like the original patch came in through the AUTOSEL
> process, and you were cc:ed on it back on July 4, 2021:
> 	https://lore.kernel.org/r/20210704230804.1490078-2-sashal@xxxxxxxxxx
> 
> In reading the changelog of the commit, and looking briefly at the
> patch, I can see why it was selected as a candidate for backporting to
> stable kernels (fixes reported problem from kernel test robot, fixes
> reference counting logic, etc.)
> 
> So given that it seemed like a normal candidate for a stable fix, and no
> one complained, after one week, it was applied to the tree on July 11
> (but Sasha's scripts did NOT email you about that for some reason,
> hopefully he has fixed that by now).
> 
> Then on July 12, I added commit 5e6b8a50a7ce ("cred: add missing return
> error code when set_cred_ucounts() failed") to the stable patch queue,
> as it fixed a reported problem in the original commit, and you were
> copied on that.
> 
> Then later that day, it was put out for review:
> 	https://lore.kernel.org/r/20210712060854.324880966@xxxxxxxxxxxxxxxxxxx
> and you were copied on that as well.
> 
> So you should have an email trail of when this patch was submitted for
> inclusion, and then put out for review.  Do you not see them in your
> email system?
> 
> 
> I just tested a local build, and yes, it can easily be reverted (along
> with a follow-on patch that was applied to resolve a problem with this
> commit), and I will queue them up after this release happens, but for
> now, I'll let this patch stay so that we do not break anyone.
> 

All now reverted.

thanks,

greg k-h