* [PATCH] CIFS should honour umask
@ 2007-06-06 19:07 Matt Keenan
2007-06-07 0:23 ` Steve French
0 siblings, 1 reply; 4+ messages in thread
From: Matt Keenan @ 2007-06-06 19:07 UTC (permalink / raw)
To: Steve French; +Cc: Jeremy Allison, LKML, linux-cifs-client
[-- Attachment #1: Type: text/plain, Size: 1344 bytes --]
This patch makes CIFS honour a process' umask like other filesystems.
Of course the server is still free to munge the permissions if it wants
to; but the client will send the "right" permissions to begin with.
A few caveats;
1) It only applies to filesystems that have CAP_UNIX (aka support unix
extensions)
2) It applies the correct mode to the follow up CIFSSMBUnixSetPerms()
after remote creation (I can write a new patch if you want with the
"right" mode at actual creation time; however the "right" perms will
still need to be given to the follow up CIFSSMBUnixSetPerms() anyway).
3) It will probably work best with Samba 3.0.25a or newer (ie with this
patch applied
http://lists.samba.org/archive/linux-cifs-client/2007-January/001697.html)
4) It has been compiled, and tested on 2.6.22-rc4 / Samba 3.0.25a
(Ubuntu Dapper with a few custom backports), and with a bit of testing
seems to work just fine. (it also incidentally side steps bugs in
thunderbird and openoffice (the apps don't check the permissions on
files they create, they assume they will open() the way that have asked
them to be created xref open(O_WRONLY|O_CREAT) => valid fd then
mmap(fd,PROT_READ) => EFAULT).
I am going to give this patch a more thorough test tomorrow with ltp.
Comments, corrections, et al are welcome.
Matt
--
Matt Keenan
OpCode Solutions
[-- Attachment #2: patch-2.6.22-rc4.cifs-umask-fix --]
[-- Type: text/plain, Size: 2239 bytes --]
Signed-off-by: Matt Keenan <matt@opcode-solutions.com>
--
diff -urN linux-2.6.22-rc4/fs/cifs/dir.c linux-2.6.22-rc4.cifs-umask-fix/fs/cifs/dir.c
--- linux-2.6.22-rc4/fs/cifs/dir.c 2007-06-06 08:34:03.000000000 +0100
+++ linux-2.6.22-rc4.cifs-umask-fix/fs/cifs/dir.c 2007-06-06 19:27:00.000000000 +0100
@@ -206,7 +206,11 @@
/* If Open reported that we actually created a file
then we now have to set the mode if possible */
if ((cifs_sb->tcon->ses->capabilities & CAP_UNIX) &&
- (oplock & CIFS_CREATE_ACTION))
+ (oplock & CIFS_CREATE_ACTION)) {
+ /* respect umask settings like other filesystems;
+ * if the server wants to munge the bits let it, but the client
+ * should "Do The Right Thing" (tm) */
+ mode &= ~current->fs->umask;
if(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID) {
CIFSSMBUnixSetPerms(xid, pTcon, full_path, mode,
(__u64)current->fsuid,
@@ -224,7 +228,7 @@
cifs_sb->mnt_cifs_flags &
CIFS_MOUNT_MAP_SPECIAL_CHR);
}
- else {
+ } else {
/* BB implement mode setting via Windows security descriptors */
/* eg CIFSSMBWinSetPerms(xid,pTcon,full_path,mode,-1,-1,local_nls);*/
/* could set r/o dos attribute if mode & 0222 == 0 */
diff -urN linux-2.6.22-rc4/fs/cifs/inode.c linux-2.6.22-rc4.cifs-umask-fix/fs/cifs/inode.c
--- linux-2.6.22-rc4/fs/cifs/inode.c 2007-06-06 08:34:03.000000000 +0100
+++ linux-2.6.22-rc4.cifs-umask-fix/fs/cifs/inode.c 2007-06-06 19:26:26.000000000 +0100
@@ -987,6 +987,11 @@
if ((direntry->d_inode) && (direntry->d_inode->i_nlink < 2))
direntry->d_inode->i_nlink = 2;
if (cifs_sb->tcon->ses->capabilities & CAP_UNIX)
+ {
+ /* respect umask settings like other filesystems;
+ * if the server wants to munge the bits let it, but the client
+ * should "Do The Right Thing" (tm) */
+ mode &= ~current->fs->umask;
if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID) {
CIFSSMBUnixSetPerms(xid, pTcon, full_path,
mode,
@@ -1004,7 +1009,7 @@
cifs_sb->mnt_cifs_flags &
CIFS_MOUNT_MAP_SPECIAL_CHR);
}
- else {
+ } else {
/* BB to be implemented via Windows secrty descriptors
eg CIFSSMBWinSetPerms(xid, pTcon, full_path, mode,
-1, -1, local_nls); */
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] CIFS should honour umask
2007-06-06 19:07 [PATCH] CIFS should honour umask Matt Keenan
@ 2007-06-07 0:23 ` Steve French
[not found] ` <4667AE70.7090800@opcode-solutions.com>
0 siblings, 1 reply; 4+ messages in thread
From: Steve French @ 2007-06-07 0:23 UTC (permalink / raw)
To: Matt Keenan; +Cc: Steve French, Jeremy Allison, LKML, linux-cifs-client
Thanks - it looks almost right but you missed mknod case and your
patch had some whitespace/formatting problems.
Could you try the following and make sure it works for you? If so will merge.
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index f085db9..8e86aac 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -208,7 +208,8 @@ cifs_create(struct inode *inode, struct
/* If Open reported that we actually created a file
then we now have to set the mode if possible */
if ((cifs_sb->tcon->ses->capabilities & CAP_UNIX) &&
- (oplock & CIFS_CREATE_ACTION))
+ (oplock & CIFS_CREATE_ACTION)) {
+ mode &= ~current->fs->umask;
if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID) {
CIFSSMBUnixSetPerms(xid, pTcon, full_path, mode,
(__u64)current->fsuid,
@@ -226,7 +227,7 @@ cifs_create(struct inode *inode, struct
cifs_sb->mnt_cifs_flags &
CIFS_MOUNT_MAP_SPECIAL_CHR);
}
- else {
+ } else {
/* BB implement mode setting via Windows security
descriptors e.g. */
/* CIFSSMBWinSetPerms(xid,pTcon,path,mode,-1,-1,nls);*/
@@ -336,6 +337,7 @@ int cifs_mknod(struct inode *inode, stru
if (full_path == NULL)
rc = -ENOMEM;
else if (pTcon->ses->capabilities & CAP_UNIX) {
+ mode &= ~current->fs->umask;
if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID) {
rc = CIFSSMBUnixSetPerms(xid, pTcon, full_path,
mode, (__u64)current->fsuid,
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 3e87dad..f0ff12b 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -986,7 +986,8 @@ mkdir_get_info:
* failed to get it from the server or was set bogus */
if ((direntry->d_inode) && (direntry->d_inode->i_nlink < 2))
direntry->d_inode->i_nlink = 2;
- if (cifs_sb->tcon->ses->capabilities & CAP_UNIX)
+ if (cifs_sb->tcon->ses->capabilities & CAP_UNIX) {
+ mode &= ~current->fs->umask;
if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID) {
CIFSSMBUnixSetPerms(xid, pTcon, full_path,
mode,
@@ -1004,7 +1005,7 @@ mkdir_get_info:
cifs_sb->mnt_cifs_flags &
CIFS_MOUNT_MAP_SPECIAL_CHR);
}
- else {
+ } else {
/* BB to be implemented via Windows secrty descriptors
eg CIFSSMBWinSetPerms(xid, pTcon, full_path, mode,
-1, -1, local_nls); */
On 6/6/07, Matt Keenan <matt@opcode-solutions.com> wrote:
> This patch makes CIFS honour a process' umask like other filesystems.
> Of course the server is still free to munge the permissions if it wants
> to; but the client will send the "right" permissions to begin with.
>
> A few caveats;
>
> 1) It only applies to filesystems that have CAP_UNIX (aka support unix
> extensions)
> 2) It applies the correct mode to the follow up CIFSSMBUnixSetPerms()
> after remote creation (I can write a new patch if you want with the
> "right" mode at actual creation time; however the "right" perms will
> still need to be given to the follow up CIFSSMBUnixSetPerms() anyway).
> 3) It will probably work best with Samba 3.0.25a or newer (ie with this
> patch applied
> http://lists.samba.org/archive/linux-cifs-client/2007-January/001697.html)
> 4) It has been compiled, and tested on 2.6.22-rc4 / Samba 3.0.25a
> (Ubuntu Dapper with a few custom backports), and with a bit of testing
> seems to work just fine. (it also incidentally side steps bugs in
> thunderbird and openoffice (the apps don't check the permissions on
> files they create, they assume they will open() the way that have asked
> them to be created xref open(O_WRONLY|O_CREAT) => valid fd then
> mmap(fd,PROT_READ) => EFAULT).
>
> I am going to give this patch a more thorough test tomorrow with ltp.
> Comments, corrections, et al are welcome.
>
>
> Matt
>
> --
> Matt Keenan
> OpCode Solutions
>
>
>
> Signed-off-by: Matt Keenan <matt@opcode-solutions.com>
>
> --
> diff -urN linux-2.6.22-rc4/fs/cifs/dir.c linux-2.6.22-rc4.cifs-umask-fix/fs/cifs/dir.c
> --- linux-2.6.22-rc4/fs/cifs/dir.c 2007-06-06 08:34:03.000000000 +0100
> +++ linux-2.6.22-rc4.cifs-umask-fix/fs/cifs/dir.c 2007-06-06 19:27:00.000000000 +0100
> @@ -206,7 +206,11 @@
> /* If Open reported that we actually created a file
> then we now have to set the mode if possible */
> if ((cifs_sb->tcon->ses->capabilities & CAP_UNIX) &&
> - (oplock & CIFS_CREATE_ACTION))
> + (oplock & CIFS_CREATE_ACTION)) {
> + /* respect umask settings like other filesystems;
> + * if the server wants to munge the bits let it, but the client
> + * should "Do The Right Thing" (tm) */
> + mode &= ~current->fs->umask;
> if(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID) {
> CIFSSMBUnixSetPerms(xid, pTcon, full_path, mode,
> (__u64)current->fsuid,
> @@ -224,7 +228,7 @@
> cifs_sb->mnt_cifs_flags &
> CIFS_MOUNT_MAP_SPECIAL_CHR);
> }
> - else {
> + } else {
> /* BB implement mode setting via Windows security descriptors */
> /* eg CIFSSMBWinSetPerms(xid,pTcon,full_path,mode,-1,-1,local_nls);*/
> /* could set r/o dos attribute if mode & 0222 == 0 */
> diff -urN linux-2.6.22-rc4/fs/cifs/inode.c linux-2.6.22-rc4.cifs-umask-fix/fs/cifs/inode.c
> --- linux-2.6.22-rc4/fs/cifs/inode.c 2007-06-06 08:34:03.000000000 +0100
> +++ linux-2.6.22-rc4.cifs-umask-fix/fs/cifs/inode.c 2007-06-06 19:26:26.000000000 +0100
> @@ -987,6 +987,11 @@
> if ((direntry->d_inode) && (direntry->d_inode->i_nlink < 2))
> direntry->d_inode->i_nlink = 2;
> if (cifs_sb->tcon->ses->capabilities & CAP_UNIX)
> + {
> + /* respect umask settings like other filesystems;
> + * if the server wants to munge the bits let it, but the client
> + * should "Do The Right Thing" (tm) */
> + mode &= ~current->fs->umask;
> if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID) {
> CIFSSMBUnixSetPerms(xid, pTcon, full_path,
> mode,
> @@ -1004,7 +1009,7 @@
> cifs_sb->mnt_cifs_flags &
> CIFS_MOUNT_MAP_SPECIAL_CHR);
> }
> - else {
> + } else {
> /* BB to be implemented via Windows secrty descriptors
> eg CIFSSMBWinSetPerms(xid, pTcon, full_path, mode,
> -1, -1, local_nls); */
>
>
--
Thanks,
Steve
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] CIFS should honour umask
[not found] ` <4667AE70.7090800@opcode-solutions.com>
@ 2007-06-07 17:01 ` Steve French
2007-06-07 17:42 ` Matt Keenan
0 siblings, 1 reply; 4+ messages in thread
From: Steve French @ 2007-06-07 17:01 UTC (permalink / raw)
To: linux-fsdevel, linux-cifs-client; +Cc: matt
For the non-unix case (e.g. Windows servers) the mode will be taken
from the default specified on the mount. I am not sure if we also
should add code to also honor umask in that case.
I am not sure how common it is to change umask to different values in
different processes which would access the same mount.
Thoughts?
> Steve French wrote:
> > Thanks - it looks almost right but you missed mknod case and your
> > patch had some whitespace/formatting problems.
> >
> > Could you try the following and make sure it works for you? If so
> > will merge.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] CIFS should honour umask
2007-06-07 17:01 ` Steve French
@ 2007-06-07 17:42 ` Matt Keenan
0 siblings, 0 replies; 4+ messages in thread
From: Matt Keenan @ 2007-06-07 17:42 UTC (permalink / raw)
To: Steve French; +Cc: linux-fsdevel, linux-cifs-client
Steve French wrote:
> For the non-unix case (e.g. Windows servers) the mode will be taken
> from the default specified on the mount. I am not sure if we also
> should add code to also honor umask in that case.
>
I don't think it would be necessary to add code for the windows case, we
should just rely on windows semantics (with acl if supplied) for pure
windows mounts.
> I am not sure how common it is to change umask to different values in
> different processes which would access the same mount.
>
I think if we are going to use umask we should use it the way it is used
in Unix, i.e. tagged to the process. I realise that in some
circumstances that having a mount wide umask would be handy, however if
we are going to apply unix style semantics we should stick to the unix
style process bound umask; i.e. the method of least suprise.
Matt
--
Matt Keenan
OpCode Solutions
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-06-07 17:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-06-06 19:07 [PATCH] CIFS should honour umask Matt Keenan
2007-06-07 0:23 ` Steve French
[not found] ` <4667AE70.7090800@opcode-solutions.com>
2007-06-07 17:01 ` Steve French
2007-06-07 17:42 ` Matt Keenan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.