Re: open_downgrade use

[Olga Kornievskaia <aglo@umich.edu>]

On Thu, Jun 2, 2016 at 3:48 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> On Wed, Jun 1, 2016 at 4:59 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>> On Wed, Jun 1, 2016 at 4:41 PM, Trond Myklebust <trondmy@primarydata.com> wrote:
>>> You are misreading what I wrote. Your test should indeed give rise to an
>>> OPEN_DOWNGRADE (unless there is a delegation involved). The code that was
>>> misbehaving and that was fixed by the patch was triggering an OPEN_DOWNGRADE
>>> from a stateid that had only been opened for RW.
>>
>> I see. With this patch, the upstream code no longer sends an
>> OPEN_DOWNGRADE. I will investigate why then as it seems like a bug.
>
> Can you please help explain the logic of this commit as my solution is
> to negate this:
>
> commit aee7af356e151494d5014f57b33460b162f181b5
> Author: Trond Myklebust <trond.myklebust@primarydata.com>
> Date:   Mon Aug 25 22:33:12 2014 -0400
>
>     NFSv4: Fix problems with close in the presence of a delegation
>
>     In the presence of delegations, we can no longer assume that the
>     state->n_rdwr, state->n_rdonly, state->n_wronly reflect the open
>     stateid share mode, and so we need to calculate the initial value
>     for calldata->arg.fmode using the state->flags.
>
>     Reported-by: James Drews <drews@engr.wisc.edu>
>     Fixes: 88069f77e1ac5 (NFSv41: Fix a potential state leakage when...)
>     Cc: stable@vger.kernel.org # 2.6.33+
>     Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>
> When close(fd0) come which suppose to translate into OPEN_DOWNGRADE:
>
> nfs4_close_prepare is_rdwr=1 is_rdonly=1 is_wronly=0 n_rdwr=0
> n_rdonly=1 n_wonly=0
>
>         if (state->n_rdwr == 0) {
>                 if (state->n_rdonly == 0)
>                         call_close |= is_rdonly;
>                 else if (is_rdonly)
>                         calldata->arg.fmode |= FMODE_READ;  <** this
> is set but call_close ends up being 0 **>
>                 if (state->n_wronly == 0)
>                         call_close |= is_wronly;
>                 else if (is_wronly)
>                         calldata->arg.fmode |= FMODE_WRITE;
>         } else if (is_rdwr)
>                 calldata->arg.fmode |= FMODE_READ|FMODE_WRITE;
>
> so then the check for !call_close later sends it to no_action and
> nothing is sent.
>
> Here's what I propose to fix it:
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 327b8c3..1db2e31 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2870,7 +2870,7 @@ static void nfs4_close_prepare(struct rpc_task *task, void
>                 call_close = 0;
>         spin_unlock(&state->owner->so_lock);
>
> -       if (!call_close) {
> +       if (!call_close && !calldata->arg.fmode) {
>                 /* Note: exit _without_ calling nfs4_close_done */
>                 goto out_no_action;
>         }
>
> But then I really don't understand why not sent call_close for if
> (is_rdonly) case?

Trond,

What do you think about the fix for the OPEN_DOWNGRADE problem?


>
> Thank you.
>>
>>>
>>>
>>> On 6/1/16, 16:31, "linux-nfs-owner@vger.kernel.org on behalf of Olga
>>> Kornievskaia" <linux-nfs-owner@vger.kernel.org on behalf of aglo@umich.edu>
>>> wrote:
>>>
>>>>I'm failing to think of what can trigger an open_downgrade?
>>>>I thought the following example should trigger an open downgrade:
>>>>
>>>>fd0 = open(foo, RDRW) -- should be open on the wire for "both"
>>>>fd1 = open(foo, RDONLY) -- should be open on the wire for "read"
>>>>close(fd0) -- should trigger an open_downgrade
>>>>read(fd1)
>>>>close(fd1)
>>>>
>>>>However this commit says that it's not allowed by the spec.
>>>>
>>>>commit cd9288ffaea4359d5cfe2b8d264911506aed26a4
>>>>Author: Trond Myklebust <trond.myklebust@primarydata.com>
>>>>Date: Thu Sep 18 11:51:32 2014 -0400
>>>>
>>>> NFSv4: Fix another bug in the close/open_downgrade code
>>>>
>>>> James Drew reports another bug whereby the NFS client is now sending
>>>> an OPEN_DOWNGRADE in a situation where it should really have sent a
>>>> CLOSE: the client is opening the file for O_RDWR, but then trying to
>>>> do a downgrade to O_RDONLY, which is not allowed by the NFSv4 spec.
>>>>
>>>> Reported-by: James Drews <drews@engr.wisc.edu>
>>>> Link: http://lkml.kernel.org/r/541AD7E5.8020409@engr.wisc.edu
>>>> Fixes: aee7af356e15 (NFSv4: Fix problems with close in the presence...)
>>>> Cc: stable@vger.kernel.org # 2.6.33+
>>>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>>>>
>>>>If RDWR to RDONLY isn't allowed then why do we have OPEN_DOWNGRADE at all?
>>>>--
>>>>To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>>>the body of a message to majordomo@vger.kernel.org
>>>>More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>>
>>> Disclaimer
>>>
>>> The information contained in this communication from the sender is
>>> confidential. It is intended solely for use by the recipient and others
>>> authorized to receive it. If you are not the recipient, you are hereby
>>> notified that any disclosure, copying, distribution or taking action in
>>> relation of the contents of this information is strictly prohibited and may
>>> be unlawful.