linux-kernel - Re: [PATCH v3 6/7] NFSv4: Add O

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:	Thu, 4 Apr 2013 14:30:12 +0400
From:	Pavel Shilovsky <piastry@...rsoft.ru>
To:	Jeff Layton <jlayton@...hat.com>
Cc:	linux-kernel@...r.kernel.org,
	linux-cifs <linux-cifs@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Linux NFS Mailing list <linux-nfs@...r.kernel.org>,
	wine-devel@...ehq.org
Subject: Re: [PATCH v3 6/7] NFSv4: Add O_DENY* open flags support

2013/3/12 Jeff Layton <jlayton@...hat.com>:
> On Mon, 11 Mar 2013 14:54:34 -0400
> Jeff Layton <jlayton@...hat.com> wrote:
>
>> On Thu, 28 Feb 2013 19:25:32 +0400
>> Pavel Shilovsky <piastry@...rsoft.ru> wrote:
>>
>> > by passing these flags to NFSv4 open request.
>> >
>> > Signed-off-by: Pavel Shilovsky <piastry@...rsoft.ru>
>> > ---
>> >  fs/nfs/nfs4xdr.c | 24 ++++++++++++++++++++----
>> >  1 file changed, 20 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>> > index 26b1439..58ddc74 100644
>> > --- a/fs/nfs/nfs4xdr.c
>> > +++ b/fs/nfs/nfs4xdr.c
>> > @@ -1325,7 +1325,8 @@ static void encode_lookup(struct xdr_stream *xdr, const struct qstr *name, struc
>> >     encode_string(xdr, name->len, name->name);
>> >  }
>> >
>> > -static void encode_share_access(struct xdr_stream *xdr, fmode_t fmode)
>> > +static void encode_share_access(struct xdr_stream *xdr, fmode_t fmode,
>> > +                           int open_flags)
>> >  {
>> >     __be32 *p;
>> >
>> > @@ -1343,7 +1344,22 @@ static void encode_share_access(struct xdr_stream *xdr, fmode_t fmode)
>> >     default:
>> >             *p++ = cpu_to_be32(0);
>> >     }
>> > -   *p = cpu_to_be32(0);            /* for linux, share_deny = 0 always */
>> > +   if (open_flags & O_DENYMAND) {
>>
>>
>> As Bruce mentioned, I think a mount option to enable this on a per-fs
>> basis would be a better approach than this new O_DENYMAND flag.
>>
>>
>> > +           switch (open_flags & (O_DENYREAD|O_DENYWRITE)) {
>> > +           case O_DENYREAD:
>> > +                   *p = cpu_to_be32(NFS4_SHARE_DENY_READ);
>> > +                   break;
>> > +           case O_DENYWRITE:
>> > +                   *p = cpu_to_be32(NFS4_SHARE_DENY_WRITE);
>> > +                   break;
>> > +           case O_DENYREAD|O_DENYWRITE:
>> > +                   *p = cpu_to_be32(NFS4_SHARE_DENY_BOTH);
>> > +                   break;
>> > +           default:
>> > +                   *p = cpu_to_be32(0);
>> > +           }
>> > +   } else
>> > +           *p = cpu_to_be32(0);
>> >  }
>> >
>> >  static inline void encode_openhdr(struct xdr_stream *xdr, const struct nfs_openargs *arg)
>> > @@ -1354,7 +1370,7 @@ static inline void encode_openhdr(struct xdr_stream *xdr, const struct nfs_opena
>> >   * owner 4 = 32
>> >   */
>> >     encode_nfs4_seqid(xdr, arg->seqid);
>> > -   encode_share_access(xdr, arg->fmode);
>> > +   encode_share_access(xdr, arg->fmode, arg->open_flags);
>> >     p = reserve_space(xdr, 36);
>> >     p = xdr_encode_hyper(p, arg->clientid);
>> >     *p++ = cpu_to_be32(24);
>> > @@ -1491,7 +1507,7 @@ static void encode_open_downgrade(struct xdr_stream *xdr, const struct nfs_close
>> >     encode_op_hdr(xdr, OP_OPEN_DOWNGRADE, decode_open_downgrade_maxsz, hdr);
>> >     encode_nfs4_stateid(xdr, arg->stateid);
>> >     encode_nfs4_seqid(xdr, arg->seqid);
>> > -   encode_share_access(xdr, arg->fmode);
>> > +   encode_share_access(xdr, arg->fmode, 0);
>> >  }
>> >
>> >  static void
>>
>>
>> Other than that, this seems reasonable.
>>
>> Acked-by: Jeff Layton <jlayton@...hat.com>
>
> Oh duh...
>
> Please ignore my comment on patch #7 to add a patch for the NFS client.
> This one does that. That said, there may be a potential problem here
> that you need to consider.
>
> In the case of a local filesystem you'll want to set deny locks using
> deny_lock_file(). For a network filesystem like CIFS or NFS though,
> the server will handle that atomically during the open. You need to
> ensure that you don't go trying to set LOCK_MAND locks on the file once
> that's done.
>
> Perhaps you can use a fstype flag to indicate that the filesystem
> handles this during the open and doesn't need to try and set a flock
> lock?

Also, we can simply mask off O_DENY* flags in open (and atomic_open)
codepath of filesystems that support these flags:

...
do open request to the storage
...
file->f_flags &= ~(O_DENYREAD | O_DENYWRITE | O_DENYDELETE);
...
return to VFS
...

Thoughts?

--
Best regards,
Pavel Shilovsky.
--
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/