linux-kernel - Re: [PATCH 2/2] NFSv4.1: Fix a race in nfs4_write

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

Date:	Fri, 17 Jan 2014 21:05:22 +0800
From:	Peng Tao <bergwolf@...il.com>
To:	Trond Myklebust <trond.myklebust@...marydata.com>
Cc:	shaobingqing <shaobingqing@...tor.com.cn>,
	linuxnfs <linux-nfs@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] NFSv4.1: Fix a race in nfs4_write_inode

On Fri, Jan 17, 2014 at 1:11 AM, Trond Myklebust
<trond.myklebust@...marydata.com> wrote:
>
> On Jan 16, 2014, at 10:49, Peng Tao <bergwolf@...il.com> wrote:
>> On Tue, Jan 14, 2014 at 2:45 AM, Trond Myklebust
>> <trond.myklebust@...marydata.com> wrote:
>>> void pnfs_set_lo_fail(struct pnfs_layout_segment *lseg)
>>> @@ -1881,43 +1887,37 @@ pnfs_layoutcommit_inode(struct inode *inode, bool sync)
>>>        struct nfs4_layoutcommit_data *data;
>>>        struct nfs_inode *nfsi = NFS_I(inode);
>>>        loff_t end_pos;
>>> -       int status = 0;
>>> +       int status;
>>>
>>> -       dprintk("--> %s inode %lu\n", __func__, inode->i_ino);
>>> -
>>> -       if (!test_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->flags))
>>> +       if (!pnfs_layoutcommit_outstanding(inode))
>> This might be a problem. If nfsi->flags has !NFS_INO_LAYOUTCOMMIT and
>> NFS_INO_LAYOUTCOMMITTING, client cannot issue a new layoutcommit after
>> the inflight one finishes. It might not be an issue for file layout as
>> long as we only use layoutcommit to update time, but it can cause data
>> corruption for block layout.
>
> I don’t understand.
>
> With the new patch, if _either_ NFS_INO_LAYOUTCOMMIT or NFS_INO_LAYOUTCOMMITTING are set, then the client will wait until NFS_INO_LAYOUTCOMMITTING can be locked, it will test for NFS_INO_LAYOUTCOMMIT, and then either issue a new layout commit or exit. How can that cause new breakage for blocks?
>
ah, sorry, I read the old code as your patch... yeah, so you actually
fixed the issue instead of introducing it.

> The only issues that I’m aware of with the blocks layout and LAYOUTCOMMIT today are:
> 1. encode_pnfs_block_layoutupdate() runs out of XDR buffer space after 4-5 iterations in the list_for_each_entry_safe() loop. That is because nobody has yet added support for preallocating a page buffer to store the (potentially very large) array of extents. BTW: that array looks like a perfect candidate for xdr_encode_array2() if we could teach the latter about xdr_stream...
This can also be fixed by limiting the number of extents allowed to be
sent in one single layoutcommit.

> 2. the blocks layout also needs to be able handle the case where the list of extents is so large that a single LAYOUTCOMMIT is not sufficient. There is no reason why it should not be able to send multiple LAYOUTCOMMIT rpc calls when the size exceeds the session forward channel's negotiated max_rqst_sz.
>
You are absolutely right.

Thanks,
Tao
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/