On Wed, Jun 15, 2016 at 10:02:41AM +1000, Dave Chinner wrote:
> On Mon, Jun 13, 2016 at 11:57:53PM +0200, Thomas D. wrote:
> > Hi,
> >
> > can anybody confirm if there's something broken with the latest XFS
> > change set which is now applied on all stable kernels?
> >
> > I found https://forums.grsecurity.net/viewtopic.php?t=4489&p=16355 and
> > grsec changelog says
> >
> > > commit 1f621dc42acbabb71bd69f6ba606cee56e7ad3bc
> > > Author: Brad Spengler <spender@xxxxxxxxxxxxxx>
> > > Date: Sat Jun 11 08:14:32 2016 -0400
> > >
> > > Fix Greg KH's broken XFS backport, caused a benign case to be detected
> > > as disk corruption
> > > Problem was due to a tree-wide conversion of error codes to their
> > > negative
> > > counterparts, which would likely never be backported to older
> > > kernels, but
> > > the backports didn't account for the change
> > >
> > > fs/xfs/xfs_inode.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Yes, the backport is busted, and needs this fix. The error sign
> change occurred in 3.17. xfstests would have picked up this
> regression in a couple of minutes, so I'm guessing that none of
> these stable releases have had any significant regression testing
> done....
>
> > This is the change grsec applied:
> >
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index fb8579d..af807d8 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -3098,7 +3111,7 @@ xfs_iflush(
> > > */
> > > error = xfs_imap_to_bp(mp, NULL, &ip->i_imap, &dip, &bp, XBF_TRYLOCK,
> > > 0);
> > > - if (error == -EAGAIN) {
> > > + if (error == EAGAIN) {
> > > xfs_ifunlock(ip);
> > > return error;
> > > }
>
> Yes, that is the fix that is needed. Thank you for reporting it to
> us.
>
> Mr Spender: it would be appreciated if you reported stable kernel
> regressions to the relevant upstream maintainers so they can be
> fixed quickly for everyone, rather than having one of your users
> decide it needs to be reported.
>
> Stable kernel maintainers: the above error sign change is needed for
> stable kernels 3.16 and earlier, as a matter of critical importance.
> And as a further matter of critical importance: in future, please
> take the time to regression test the changes you backport.
>
> > The bad commit according to grsec's statement is
> >
> > > From b1438f477934f5a4d5a44df26f3079a7575d5946 Mon Sep 17 00:00:00 2001
> > > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > > Date: Wed, 18 May 2016 13:53:42 +1000
> > > Subject: [PATCH] xfs: xfs_iflush_cluster fails to abort on error
> >
> > Would be nice to get some clarification.
>
> There's nothing wrong with that commit in the upstream kernel,
> it's the backport that has a bug in it because it failed to take
> into account changes outside the context of the upstream commit that
> the older kernels don't have.
Thanks for letting me know about this.
As the patch was tagged with 3.10+, I assumed that it was safe to be
merged to those older kernels, otherwise I would never have done so. We
do have ways to mark external things like this for stable patches, it's
a great help when doing backports.
thanks,
greg k-h
|