open_downgrade use

open_downgrade use

[Olga Kornievskaia]

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?

Re: open_downgrade use

[Trond Myklebust]

WW91IGFyZSBtaXNyZWFkaW5nIHdoYXQgSSB3cm90ZS4gWW91ciB0ZXN0IHNob3VsZCBpbmRlZWQg
Z2l2ZSByaXNlIHRvIGFuIE9QRU5fRE9XTkdSQURFICh1bmxlc3MgdGhlcmUgaXMgYSBkZWxlZ2F0
aW9uIGludm9sdmVkKS4gVGhlIGNvZGUgdGhhdCB3YXMgbWlzYmVoYXZpbmcgYW5kIHRoYXQgd2Fz
IGZpeGVkIGJ5IHRoZSBwYXRjaCB3YXMgdHJpZ2dlcmluZyBhbiBPUEVOX0RPV05HUkFERSBmcm9t
IGEgc3RhdGVpZCB0aGF0IGhhZCBvbmx5IGJlZW4gb3BlbmVkIGZvciBSVy4NCg0KT24gNi8xLzE2
LCAxNjozMSwgImxpbnV4LW5mcy1vd25lckB2Z2VyLmtlcm5lbC5vcmcgb24gYmVoYWxmIG9mIE9s
Z2EgS29ybmlldnNrYWlhIiA8bGludXgtbmZzLW93bmVyQHZnZXIua2VybmVsLm9yZyBvbiBiZWhh
bGYgb2YgYWdsb0B1bWljaC5lZHU+IHdyb3RlOg0KDQo+SSdtIGZhaWxpbmcgdG8gdGhpbmsgb2Yg
d2hhdCBjYW4gdHJpZ2dlciBhbiBvcGVuX2Rvd25ncmFkZT8NCj5JIHRob3VnaHQgdGhlIGZvbGxv
d2luZyBleGFtcGxlIHNob3VsZCB0cmlnZ2VyIGFuIG9wZW4gZG93bmdyYWRlOg0KPg0KPmZkMCA9
IG9wZW4oZm9vLCBSRFJXKSAgIC0tIHNob3VsZCBiZSBvcGVuIG9uIHRoZSB3aXJlIGZvciAiYm90
aCINCj5mZDEgPSBvcGVuKGZvbywgUkRPTkxZKSAgLS0gc2hvdWxkIGJlIG9wZW4gb24gdGhlIHdp
cmUgZm9yICJyZWFkIg0KPmNsb3NlKGZkMCkgLS0gc2hvdWxkIHRyaWdnZXIgYW4gb3Blbl9kb3du
Z3JhZGUNCj5yZWFkKGZkMSkNCj5jbG9zZShmZDEpDQo+DQo+SG93ZXZlciB0aGlzIGNvbW1pdCBz
YXlzIHRoYXQgaXQncyBub3QgYWxsb3dlZCBieSB0aGUgc3BlYy4NCj4NCj5jb21taXQgY2Q5Mjg4
ZmZhZWE0MzU5ZDVjZmUyYjhkMjY0OTExNTA2YWVkMjZhNA0KPkF1dGhvcjogVHJvbmQgTXlrbGVi
dXN0IDx0cm9uZC5teWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tPg0KPkRhdGU6ICAgVGh1IFNlcCAx
OCAxMTo1MTozMiAyMDE0IC0wNDAwDQo+DQo+ICAgIE5GU3Y0OiBGaXggYW5vdGhlciBidWcgaW4g
dGhlIGNsb3NlL29wZW5fZG93bmdyYWRlIGNvZGUNCj4NCj4gICAgSmFtZXMgRHJldyByZXBvcnRz
IGFub3RoZXIgYnVnIHdoZXJlYnkgdGhlIE5GUyBjbGllbnQgaXMgbm93IHNlbmRpbmcNCj4gICAg
YW4gT1BFTl9ET1dOR1JBREUgaW4gYSBzaXR1YXRpb24gd2hlcmUgaXQgc2hvdWxkIHJlYWxseSBo
YXZlIHNlbnQgYQ0KPiAgICBDTE9TRTogdGhlIGNsaWVudCBpcyBvcGVuaW5nIHRoZSBmaWxlIGZv
ciBPX1JEV1IsIGJ1dCB0aGVuIHRyeWluZyB0bw0KPiAgICBkbyBhIGRvd25ncmFkZSB0byBPX1JE
T05MWSwgd2hpY2ggaXMgbm90IGFsbG93ZWQgYnkgdGhlIE5GU3Y0IHNwZWMuDQo+DQo+ICAgIFJl
cG9ydGVkLWJ5OiBKYW1lcyBEcmV3cyA8ZHJld3NAZW5nci53aXNjLmVkdT4NCj4gICAgTGluazog
aHR0cDovL2xrbWwua2VybmVsLm9yZy9yLzU0MUFEN0U1LjgwMjA0MDlAZW5nci53aXNjLmVkdQ0K
PiAgICBGaXhlczogYWVlN2FmMzU2ZTE1IChORlN2NDogRml4IHByb2JsZW1zIHdpdGggY2xvc2Ug
aW4gdGhlIHByZXNlbmNlLi4uKQ0KPiAgICBDYzogc3RhYmxlQHZnZXIua2VybmVsLm9yZyAjIDIu
Ni4zMysNCj4gICAgU2lnbmVkLW9mZi1ieTogVHJvbmQgTXlrbGVidXN0IDx0cm9uZC5teWtsZWJ1
c3RAcHJpbWFyeWRhdGEuY29tPg0KPg0KPklmIFJEV1IgdG8gUkRPTkxZIGlzbid0IGFsbG93ZWQg
dGhlbiB3aHkgZG8gd2UgaGF2ZSBPUEVOX0RPV05HUkFERSBhdCBhbGw/DQo+LS0NCj5UbyB1bnN1
YnNjcmliZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUgbGludXgt
bmZzIiBpbg0KPnRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJuZWwu
b3JnDQo+TW9yZSBtYWpvcmRvbW8gaW5mbyBhdCAgaHR0cDovL3ZnZXIua2VybmVsLm9yZy9tYWpv
cmRvbW8taW5mby5odG1sDQo+DQoNCg==

Re: open_downgrade use

[Olga Kornievskaia]

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.

>
>
> 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.

RE: open_downgrade use

[Frank Filz]

> 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.

Does the client send an open for the read only? If not, per spec, having opened read/write, you can't downgrade to read only.

The server should allow the 2nd open for read only as an "upgrade" (which won't actually do anything, except allow the subsequent downgrade to read only).

Such a scenario should be allowed by Ganesha, though my algorithm isn't perfect, when downgrading to read only, we don't "forget" that the file had been opened read/write, so Ganesha would allow the following sequence:

Open read only
Open write only (upgrades to read/write)
Downgrade to read only
Open read/write (upgrades to read/write)
Downgrade to write only

That last downgrade should be rejected per the RFC.

Frank



---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

Re: open_downgrade use

[Olga Kornievskaia]

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?

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.

Re: open_downgrade use

[Olga Kornievskaia]

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.