The access denied message in the dmesg log reveals no more information than strace on stat of a local file does (which also returns access denied and displays access denied), but I agree that logging on -EACCESS on lookup does clutter the log.
I think it is ok to log a message on unexpected errors (for QueryPathInfo those would include anything other than ENOENT and EACCESS - Simo, can you think of others?) I don't mind ratelimiting logging on this clause (for errors other than ENOENT and EACCESS) but it would complicate the code for a case I have not seen in the wild. I prefer the following to remove the log cluttering on this case: diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c index 37dc97a..b2802e5 100644 --- a/fs/cifs/dir.c +++ b/fs/cifs/dir.c @@ -517,12 +517,11 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, d_add(direntry, NULL); /* if it was once a directory (but how can we tell?) we could do shrink_dcache_parent(direntry); */ - } else { + } else if (rc != -EACCES) { cERROR(1, ("Error 0x%x on cifs_get_inode_info in lookup of %s", rc, full_path)); - /* BB special case check for Access Denied - watch security - exposure of returning dir info implicitly via different rc - if file exists or not but no access BB */ + /* We special case check for Access Denied - since that + is a common return code */ } kfree(full_path); On Jan 19, 2008 2:18 AM, simo <[EMAIL PROTECTED]> wrote: > > > On Sat, 2008-01-19 at 05:55 +0100, Andi Kleen wrote: > > Fix information leak in CIFS client lookup > > > > Putting arbitary file names on lookup failures into the system log is not > > a good idea, because usually everybody can read dmesg and that is thus > > an information leak if a directory was read protected. > > > > Also changed the error printout for this case to a signed number, because > > it is normally negative and that makes it easier to read. > > > > I'm not sure the message is all that useful anyways. Perhaps it > > should be just removed completely? Or at least rate limited because > > it allows to spam the kernel log nicely. > > > > Signed-off-by: Andi Kleen <[EMAIL PROTECTED]> > > > > Index: linux/fs/cifs/dir.c > > =================================================================== > > --- linux.orig/fs/cifs/dir.c > > +++ linux/fs/cifs/dir.c > > @@ -518,7 +518,7 @@ cifs_lookup(struct inode *parent_dir_ino > > /* if it was once a directory (but how can we tell?) we could do > > shrink_dcache_parent(direntry); */ > > } else { > > - cERROR(1, ("Error 0x%x on cifs_get_inode_info in lookup of > > %s", > > + cERROR(1, ("Error %d on cifs_get_inode_info in lookup of > > file", > > rc, full_path)); > > then please remove also full_path here ^^^^ > > Simo. > > -- > Simo Sorce > Samba Team GPL Compliance Officer <[EMAIL PROTECTED]> > Senior Software Engineer at Red Hat Inc. <[EMAIL PROTECTED]> > > -- Thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/