linux-kernel - Re: [PATCH v2] xfs: handle register

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:   Sun, 26 Nov 2017 10:34:53 +1100
From:   Dave Chinner <david@...morbit.com>
To:     Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc:     mhocko@...nel.org, hch@...radead.org, darrick.wong@...cle.com,
        linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] xfs: handle register_shrinker error

On Fri, Nov 24, 2017 at 09:03:28PM +0900, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > Thanks. Updated patch below
> > ---
> > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@...e.com>
> > Date: Thu, 23 Nov 2017 17:13:40 +0100
> > Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling
> 
> Do we need below patch on top of Michal's patch?
> KM_NOFS was added by commit b17cb364dbbbf65a ("xfs: fix missing KM_NOFS
> tags to keep lockdep happy"). If not needed, some comment is expected.

Quite frankly, if the fix is "sprinkle magic undocumented
memalloc_nofs_save() calls around", then you need to think a little
more about the things you just read and the context we're operating
on here.

IOWs:

> ---
>  fs/xfs/xfs_buf.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 4c6e86d..b73fc76 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1806,6 +1806,7 @@ struct xfs_buf *
>  	struct dax_device	*dax_dev)
>  {
>  	xfs_buftarg_t		*btp;
> +	unsigned int nofs_flag = memalloc_nofs_save();
>  
>  	btp = kmem_zalloc(sizeof(*btp), KM_SLEEP | KM_NOFS);

xfs_alloc_buftarg() isn't called from transaction context, so this
KM_NOFS flag wasn't added to prevent reclaim deadlocks - it was
added to avoid stupid lockdep false positives (as was stated in the
commit you quoted).

IOWs, GFP_KERNEL allocations in this function used to trigger
lockdep false positives.

So - think for a minute rather than bashing on the keyboard. Why
aren't the other GFP_KERNEL allocations from this function causing
lockdep to trigger warnings?

Yeah - lockdep is a lot smarter these days and the false positive
trigger has clearly been fixed. i.e. there's no false positive
detection occurring here any more under GFP_KERNEL allocations,
so we don't need the KM_NOFS flag anymore.

IOWs, we don't actually need to touch this code, but if you really
must, just remove the KM_NOFS tag.

-Dave.

-- 
Dave Chinner
david@...morbit.com