On Mon, 10 Sep 2012 07:56:36 -0500
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Mon, Sep 10, 2012 at 7:23 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
>
> > On Sun, 9 Sep 2012 19:31:16 -0500
> > Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> >
> > > i
> > >
> > > On Wed, Sep 5, 2012 at 9:11 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> > >
> > > > On Mon, 3 Sep 2012 18:06:06 -0500
> > > > shirishpargaonkar@xxxxxxxxx wrote:
> > > >
> > > > > From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> > > > >
> > > > >
> > > > > Add support of Alternate Data Streams (ads).
> > > > > For the support, we need to add additional (desired) access flags
> > > > > to the generic ones that cifs client does currently.
> > > > > The ads files have a : in the name, so that is used to differentiate
> > > > > and add additional desired access.
> > > > > One operations that does not work is Rename (0x7).
> > > > >
> > > > >
> > > > > Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> > > > > ---
> > > > > fs/cifs/dir.c | 12 ++++++++++--
> > > > > fs/cifs/file.c | 45 ++++++++++++++++++++++++++++++++++-----------
> > > > > fs/cifs/inode.c | 14 ++++++++++++++
> > > > > 3 files changed, 58 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > > > > index cbe709a..8f8d546 100644
> > > > > --- a/fs/cifs/dir.c
> > > > > +++ b/fs/cifs/dir.c
> > > > > @@ -245,10 +245,18 @@ cifs_do_create(struct inode *inode, struct
> > dentry
> > > > *direntry, unsigned int xid,
> > > > > }
> > > > >
> > > > > desiredAccess = 0;
> > > > > - if (OPEN_FMODE(oflags) & FMODE_READ)
> > > > > + if (OPEN_FMODE(oflags) & FMODE_READ) {
> > > > > desiredAccess |= GENERIC_READ; /* is this too little?
> > */
> > > > > - if (OPEN_FMODE(oflags) & FMODE_WRITE)
> > > > > + if (strstr(full_path, ":"))
> > > > > + desiredAccess |= FILE_READ_DATA | FILE_READ_EA
> > |
> > > > > + FILE_READ_ATTRIBUTES | READ_CONTROL;
> > > > > + }
> > > > > + if (OPEN_FMODE(oflags) & FMODE_WRITE) {
> > > > > desiredAccess |= GENERIC_WRITE;
> > > > > + if (strstr(full_path, ":"))
> > > > > + desiredAccess |= FILE_WRITE_DATA |
> > > > FILE_APPEND_DATA |
> > > > > + FILE_WRITE_EA |
> > > > FILE_WRITE_ATTRIBUTES;
> > > > > + }
> > > > >
> > > > > disposition = FILE_OVERWRITE_IF;
> > > > > if ((oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
> > > > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > > > > index 9154192..80f35f8 100644
> > > > > --- a/fs/cifs/file.c
> > > > > +++ b/fs/cifs/file.c
> > > > > @@ -43,22 +43,41 @@
> > > > > #include "cifs_fs_sb.h"
> > > > > #include "fscache.h"
> > > > >
> > > > > -static inline int cifs_convert_flags(unsigned int flags)
> > > > > -{
> > > > > - if ((flags & O_ACCMODE) == O_RDONLY)
> > > > > - return GENERIC_READ;
> > > > > - else if ((flags & O_ACCMODE) == O_WRONLY)
> > > > > - return GENERIC_WRITE;
> > > > > - else if ((flags & O_ACCMODE) == O_RDWR) {
> > > > > +static inline int cifs_convert_flags(char *full_path, unsigned int
> > > > flags)
> > > > > +{
> > > > > + int daccess = 0;
> > > > > +
> > > > > + if ((flags & O_ACCMODE) == O_RDONLY) {
> > > > > + daccess = GENERIC_READ;
> > > > > + if (strstr(full_path, ":"))
> > > > > + daccess |= FILE_READ_DATA | FILE_READ_EA |
> > > > > + FILE_READ_ATTRIBUTES | READ_CONTROL;
> > > > > + return daccess;
> > > > > + } else if ((flags & O_ACCMODE) == O_WRONLY) {
> > > > > + daccess = GENERIC_WRITE;
> > > > > + if (strstr(full_path, ":"))
> > > > > + daccess |= FILE_WRITE_DATA | FILE_APPEND_DATA |
> > > > > + FILE_WRITE_EA | FILE_WRITE_ATTRIBUTES;
> > > > > + return daccess;
> > > > > + } else if ((flags & O_ACCMODE) == O_RDWR) {
> > > > > /* GENERIC_ALL is too much permission to request
> > > > > can cause unnecessary access denied on create */
> > > > > /* return GENERIC_ALL; */
> > > > > - return (GENERIC_READ | GENERIC_WRITE);
> > > > > + daccess = GENERIC_READ | GENERIC_WRITE;
> > > > > + if (strstr(full_path, ":"))
> > > > > + daccess |= FILE_READ_DATA | FILE_WRITE_DATA |
> > > > > + FILE_APPEND_DATA | FILE_READ_EA |
> > > > > + FILE_WRITE_EA | FILE_READ_ATTRIBUTES |
> > > > > + FILE_WRITE_ATTRIBUTES | READ_CONTROL;
> > > > > + return daccess;
> > > >
> > > >
> > > > So if any component in the pathname happens to have a ':' in it, then
> > > > we'll open the file with a different set of access flags? ':' is a
> > > > perfectly legitimate character in posix pathnames. That sounds like a
> > > > very bad heuristic.
> > > >
> > > > > }
> > > > >
> > > > > - return (READ_CONTROL | FILE_WRITE_ATTRIBUTES |
> > > > FILE_READ_ATTRIBUTES |
> > > > > + daccess = READ_CONTROL | FILE_WRITE_ATTRIBUTES |
> > > > FILE_READ_ATTRIBUTES |
> > > > > FILE_WRITE_EA | FILE_APPEND_DATA | FILE_WRITE_DATA |
> > > > > - FILE_READ_DATA);
> > > > > + FILE_READ_DATA;
> > > > > +
> > > > > + return daccess;
> > > > > +
> > > > > }
> > > > >
> > > > > static u32 cifs_posix_convert_flags(unsigned int flags)
> > > > > @@ -149,6 +168,8 @@ int cifs_posix_open(char *full_path, struct inode
> > > > **pinode,
> > > > > goto posix_open_ret; /* caller does not need info */
> > > > >
> > > > > cifs_unix_basic_to_fattr(&fattr, presp_data, cifs_sb);
> > > > > + if (strstr(full_path, ":"))
> > > > > + fattr.cf_flags |= S_PRIVATE;
> > > > >
> > > > > /* get new inode and set it up */
> > > > > if (*pinode == NULL) {
> > > > > @@ -158,6 +179,8 @@ int cifs_posix_open(char *full_path, struct inode
> > > > **pinode,
> > > > > rc = -ENOMEM;
> > > > > goto posix_open_ret;
> > > > > }
> > > > > + if (strstr(full_path, ":"))
> > > > > + (*pinode)->i_flags |= S_PRIVATE;
> > > > > } else {
> > > > > cifs_fattr_to_inode(*pinode, &fattr);
> > > > > }
> > > > > @@ -178,7 +201,7 @@ cifs_nt_open(char *full_path, struct inode
> > *inode,
> > > > struct cifs_sb_info *cifs_sb,
> > > > > int create_options = CREATE_NOT_DIR;
> > > > > FILE_ALL_INFO *buf;
> > > > >
> > > > > - desiredAccess = cifs_convert_flags(f_flags);
> > > > > + desiredAccess = cifs_convert_flags(full_path, f_flags);
> > > > >
> > > > >
> > /*********************************************************************
> > > > > * open flag mapping table:
> > > > > @@ -538,7 +561,7 @@ static int cifs_reopen_file(struct cifsFileInfo
> > > > *pCifsFile, bool can_flush)
> > > > > in the reconnect path it is important to retry hard
> > */
> > > > > }
> > > > >
> > > > > - desiredAccess = cifs_convert_flags(pCifsFile->f_flags);
> > > > > + desiredAccess = cifs_convert_flags(full_path,
> > pCifsFile->f_flags);
> > > > >
> > > > > if (backup_cred(cifs_sb))
> > > > > create_options |= CREATE_OPEN_BACKUP_INTENT;
> > > > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > > > > index 7354877..0bde128 100644
> > > > > --- a/fs/cifs/inode.c
> > > > > +++ b/fs/cifs/inode.c
> > > > > @@ -351,12 +351,17 @@ int cifs_get_inode_info_unix(struct inode
> > **pinode,
> > > > > cFYI(1, "CIFSCheckMFSymlink: %d", tmprc);
> > > > > }
> > > > >
> > > > > + if (strstr(full_path, ":"))
> > > > > + fattr.cf_flags |= S_PRIVATE;
> > > > > +
> > > > > if (*pinode == NULL) {
> > > > > /* get new inode */
> > > > > cifs_fill_uniqueid(sb, &fattr);
> > > > > *pinode = cifs_iget(sb, &fattr);
> > > > > if (!*pinode)
> > > > > rc = -ENOMEM;
> > > > > + if (strstr(full_path, ":"))
> > > > > + (*pinode)->i_flags |= S_PRIVATE;
> > > > > } else {
> > > > > /* we already have inode, update it */
> > > > > cifs_fattr_to_inode(*pinode, &fattr);
> > > > > @@ -713,10 +718,15 @@ cifs_get_inode_info(struct inode **inode, const
> > > > char *full_path,
> > > > > cFYI(1, "CIFSCheckMFSymlink: %d", tmprc);
> > > > > }
> > > > >
> > > > > + if (strstr(full_path, ":"))
> > > > > + fattr.cf_flags |= S_PRIVATE;
> > > > > +
> > > > > if (!*inode) {
> > > > > *inode = cifs_iget(sb, &fattr);
> > > > > if (!*inode)
> > > > > rc = -ENOMEM;
> > > > > + if (strstr(full_path, ":"))
> > > > > + (*inode)->i_flags |= S_PRIVATE;
> > > > > } else {
> > > > > cifs_fattr_to_inode(*inode, &fattr);
> > > > > }
> > > > > @@ -748,6 +758,10 @@ cifs_find_inode(struct inode *inode, void
> > *opaque)
> > > > > if ((inode->i_mode & S_IFMT) != (fattr->cf_mode & S_IFMT))
> > > > > return 0;
> > > > >
> > > > > + /* don't match inode with different flags */
> > > > > + if ((inode->i_flags & S_PRIVATE) != (fattr->cf_flags &
> > S_PRIVATE))
> > > > > + return 0;
> > > > > +
> > > > > /* if it's not a directory or has no dentries, then flag it */
> > > > > if (S_ISDIR(inode->i_mode) && !hlist_empty(&inode->i_dentry))
> > > > > fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
> > > >
> > > >
> > > > --
> > > > Jeff Layton <jlayton@xxxxxxxxx>
> > > >
> > >
> > > I think then this check for a : in the file name should be limited
> > > to non-posix_compliant cifs/smb servers like Windows?
> > > I do not think there is any way in the cifs protocol to ask whether
> > > a file object is an alternate data stream or not.
> > > A client should be oblivious to how and where a file object is
> > > stored as long as server honours its request.
> > >
> >
> > I think it's best to avoid "hidden" interfaces that change client
> > behavior based on the filename. If the only thing that's missing is to
> > request extra access flags, is there any reason not to request them
> > unconditionally?
> >
> > --
> > Jeff Layton <jlayton@xxxxxxxxx>
> >
>
> Agree. There is no reason not to request these extra access flags
> unconditionally except that would be there be a case where access
> would be denied with the extra access flags which otherwise would
> be granted without extra access flags (i.e. with the current flag(s)).
>
> At least in case of Windows servers, a regular file and alternate
> data stream have same file id. So the only way to distinguish them
> is by a : in a file name. I do not see any other way to distinguish
> them on the client.
>
Can you clarify why extra access flags are needed in this case? What's
flags are missing and why are they needed?
--
Jeff Layton <jlayton@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
