linux-kernel - Re: [PATCH v6 13/15] xfs: communicate lock drop events from xfs_break

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]

Message-ID: <20180316190801.GG3671@lst.de>
Date:   Fri, 16 Mar 2018 20:08:01 +0100
From:   Christoph Hellwig <hch@....de>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     linux-nvdimm@...ts.01.org,
        "Darrick J. Wong" <darrick.wong@...cle.com>,
        Ross Zwisler <ross.zwisler@...ux.intel.com>,
        Dave Chinner <david@...morbit.com>,
        Christoph Hellwig <hch@....de>, linux-xfs@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, jack@...e.cz,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 13/15] xfs: communicate lock drop events from
        xfs_break_layouts()

On Thu, Mar 15, 2018 at 08:52:39AM -0700, Dan Williams wrote:
> In preparation for adding a new layout type, teach xfs_break_layouts()
> to return a positive number if it needed to drop locks while trying to
> break leases. For all layouts to be successfully broken each layout type
> needs to be able to assert that the layouts were broken with the locks
> held.
> 
> The existing a xfs_break_layouts() is pushed down a level to
> xfs_break_leased_layouts() and the new xfs_break_layouts() will
> coordinate interpreting the return code from the low level 'break'
> helpers.

With that the subject line is rather confusing, given that the
externally visible xfs_break_layouts does not communicate the lock
drop events.  So maybe this should just be titled something about
refactoring.  Or just merged into the next patch which reshuffles
everything again anyway.

>  int
> -xfs_break_layouts(
> +xfs_break_leased_layouts(
>  	struct inode		*inode,
>  	uint			*iolock)
>  {
>  	struct xfs_inode	*ip = XFS_I(inode);
>  	int			error;
> -
> -	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL
> -				| XFS_MMAPLOCK_EXCL));
> +	int			did_unlock = 0;
>  
>  	while ((error = break_layout(inode, false) == -EWOULDBLOCK)) {
>  		xfs_iunlock(ip, *iolock);
> +		did_unlock = 1;
>  		error = break_layout(inode, true);
>  		*iolock &= ~XFS_IOLOCK_SHARED;
>  		*iolock |= XFS_IOLOCK_EXCL;
>  		xfs_ilock(ip, *iolock);
>  	}
>  
> -	return error;
> +	if (error < 0)
> +		return error;
> +	return did_unlock;

And I suspect the cleaner interface would be to just pass a 
bool *did_unlock argument.