[PATCH 0/13] sysfs lazification.

[Eric W. Biederman]

The sysfs code updates the vfs caches immediately when the sysfs data
structures change causing a lot of unnecessary complications.  The
following patchset untangles that beast.  Allowing for simpler
more straight forward code, the removal of a hack from the vfs
to support sysfs, and human comprehensible locking on sysfs.

Most of these patches have already been reviewed and acked from the
last time I had time to work on sysfs.

In net the patches look like:

 fs/namei.c            |   22 ---
 fs/sysfs/dir.c        |  388 ++++++++++++++++---------------------------------
 fs/sysfs/file.c       |   41 +----
 fs/sysfs/inode.c      |  178 ++++++++++++++---------
 fs/sysfs/symlink.c    |   11 +-
 fs/sysfs/sysfs.h      |    9 +-
 include/linux/namei.h |    1 -
 7 files changed, 256 insertions(+), 394 deletions(-)

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@...>

The sysfs_mutex is required to ensure updates are and will remain
atomic with respect to other inode iattr updates, that do not happen
through the filesystem.

Signed-off-by: Eric W. Biederman <ebiederm@...>
---
 fs/sysfs/inode.c |   41 +++++++++++++++++++++++++++++------------
 1 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index e28cecf..8a08250 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -122,23 +122,39 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
  return error;
 }
 
+static int sysfs_sd_setsecdata(struct sysfs_dirent *sd, void **secdata, u32 *secdata_len)
+{
+ struct sysfs_inode_attrs *iattrs;
+ void *old_secdata;
+ size_t old_secdata_len;
+
+ iattrs = sd->s_iattr;
+ if (!iattrs)
+ iattrs = sysfs_init_inode_attrs(sd);
+ if (!iattrs)
+ return -ENOMEM;
+
+ old_secdata = iattrs->ia_secdata;
+ old_secdata_len = iattrs->ia_secdata_len;
+
+ iattrs->ia_secdata = *secdata;
+ iattrs->ia_secdata_len = *secdata_len;
+
+ *secdata = old_secdata;
+ *secdata_len = old_secdata_len;
+ return 0;
+}
+
 int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
  size_t size, int flags)
 {
  struct sysfs_dirent *sd = dentry->d_fsdata;
- struct sysfs_inode_attrs *iattrs;
  void *secdata;
  int error;
  u32 secdata_len = 0;
 
  if (!sd)
  return -EINVAL;
- if (!sd->s_iattr)
- sd->s_iattr = sysfs_init_inode_attrs(sd);
- if (!sd->s_iattr)
- return -ENOMEM;
-
- iattrs = sd->s_iattr;
 
  if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
  const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
@@ -150,12 +166,13 @@ int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
  &secdata, &secdata_len);
  if (error)
  goto out;
- if (iattrs->ia_secdata)
- security_release_secctx(iattrs->ia_secdata,
- iattrs->ia_secdata_len);
- iattrs->ia_secdata = secdata;
- iattrs->ia_secdata_len = secdata_len;
 
+ mutex_lock(&sysfs_mutex);
+ error = sysfs_sd_setsecdata(sd, &secdata, &secdata_len);
+ mutex_unlock(&sysfs_mutex);
+
+ if (secdata)
+ security_release_secctx(secdata, secdata_len);
  } else
  return -EINVAL;
 out:
--
1.6.5.2.143.g8cc62

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@...>

Using dentry instead of d in the function name is what
several other filesystems are doing and it seems to be
a more readable convention.

Acked-by: Tejun Heo <tj@...>
Signed-off-by: Eric W. Biederman <ebiederm@...>
---
 fs/sysfs/dir.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index e020183..130dfc3 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -298,7 +298,7 @@ void release_sysfs_dirent(struct sysfs_dirent * sd)
  goto repeat;
 }
 
-static void sysfs_d_iput(struct dentry * dentry, struct inode * inode)
+static void sysfs_dentry_iput(struct dentry * dentry, struct inode * inode)
 {
  struct sysfs_dirent * sd = dentry->d_fsdata;
 
@@ -307,7 +307,7 @@ static void sysfs_d_iput(struct dentry * dentry, struct inode * inode)
 }
 
 static const struct dentry_operations sysfs_dentry_ops = {
- .d_iput = sysfs_d_iput,
+ .d_iput = sysfs_dentry_iput,
 };
 
 struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type)
--
1.6.5.2.143.g8cc62

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@...>

Calling d_drop unconditionally when a sysfs_dirent is deleted has
the potential to leak mounts, so instead implement dentry delete
and revalidate operations that cause sysfs dentries to be removed
at the appropriate time.

Acked-by: Tejun Heo <tj@...>
Signed-off-by: Eric W. Biederman <ebiederm@...>
---
 fs/sysfs/dir.c |   73 +++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 46 insertions(+), 27 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 130dfc3..b5e8499 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -298,6 +298,46 @@ void release_sysfs_dirent(struct sysfs_dirent * sd)
  goto repeat;
 }
 
+static int sysfs_dentry_delete(struct dentry *dentry)
+{
+ struct sysfs_dirent *sd = dentry->d_fsdata;
+ return !!(sd->s_flags & SYSFS_FLAG_REMOVED);
+}
+
+static int sysfs_dentry_revalidate(struct dentry *dentry, struct nameidata *nd)
+{
+ struct sysfs_dirent *sd = dentry->d_fsdata;
+ int is_dir;
+
+ mutex_lock(&sysfs_mutex);
+
+ /* The sysfs dirent has been deleted */
+ if (sd->s_flags & SYSFS_FLAG_REMOVED)
+ goto out_bad;
+
+ mutex_unlock(&sysfs_mutex);
+out_valid:
+ return 1;
+out_bad:
+ /* Remove the dentry from the dcache hashes.
+ * If this is a deleted dentry we use d_drop instead of d_delete
+ * so sysfs doesn't need to cope with negative dentries.
+ */
+ is_dir = (sysfs_type(sd) == SYSFS_DIR);
+ mutex_unlock(&sysfs_mutex);
+ if (is_dir) {
+ /* If we have submounts we must allow the vfs caches
+ * to lie about the state of the filesystem to prevent
+ * leaks and other nasty things.
+ */
+ if (have_submounts(dentry))
+ goto out_valid;
+ shrink_dcache_parent(dentry);
+ }
+ d_drop(dentry);
+ return 0;
+}
+
 static void sysfs_dentry_iput(struct dentry * dentry, struct inode * inode)
 {
  struct sysfs_dirent * sd = dentry->d_fsdata;
@@ -307,6 +347,8 @@ static void sysfs_dentry_iput(struct dentry * dentry, struct inode * inode)
 }
 
 static const struct dentry_operations sysfs_dentry_ops = {
+ .d_revalidate = sysfs_dentry_revalidate,
+ .d_delete = sysfs_dentry_delete,
  .d_iput = sysfs_dentry_iput,
 };
 
@@ -527,44 +569,21 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 }
 
 /**
- * sysfs_drop_dentry - drop dentry for the specified sysfs_dirent
+ * sysfs_dec_nlink - Decrement link count for the specified sysfs_dirent
  * @sd: target sysfs_dirent
  *
- * Drop dentry for @sd.  @sd must have been unlinked from its
+ * Decrement nlink for @sd.  @sd must have been unlinked from its
  * parent on entry to this function such that it can't be looked
  * up anymore.
  */
-static void sysfs_drop_dentry(struct sysfs_dirent *sd)
+static void sysfs_dec_nlink(struct sysfs_dirent *sd)
 {
  struct inode *inode;
- struct dentry *dentry;
 
  inode = ilookup(sysfs_sb, sd->s_ino);
  if (!inode)
  return;
 
- /* Drop any existing dentries associated with sd.
- *
- * For the dentry to be properly freed we need to grab a
- * reference to the dentry under the dcache lock,  unhash it,
- * and then put it.  The playing with the dentry count allows
- * dput to immediately free the dentry  if it is not in use.
- */
-repeat:
- spin_lock(&dcache_lock);
- list_for_each_entry(dentry, &inode->i_dentry, d_alias) {
- if (d_unhashed(dentry))
- continue;
- dget_locked(dentry);
- spin_lock(&dentry->d_lock);
- __d_drop(dentry);
- spin_unlock(&dentry->d_lock);
- spin_unlock(&dcache_lock);
- dput(dentry);
- goto repeat;
- }
- spin_unlock(&dcache_lock);
-
  /* adjust nlink and update timestamp */
  mutex_lock(&inode->i_mutex);
 
@@ -611,7 +630,7 @@ void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
  acxt->removed = sd->s_sibling;
  sd->s_sibling = NULL;
 
- sysfs_drop_dentry(sd);
+ sysfs_dec_nlink(sd);
  sysfs_deactivate(sd);
  unmap_bin_file(sd);
  sysfs_put(sd);
--
1.6.5.2.143.g8cc62

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@...>

Currently every caller of sysfs_chmod_file happens at either
file creation time to set a non-default mode or in response
to a specific user requested space change in policy.  Making
timestamps of when the chmod happens and notification of
a file changing mode uninteresting.

Remove the unnecessary time stamp and filesystem change
notification, and removes the last of the explicit inotify
and donitfy support from sysfs.

Acked-by: Tejun Heo <tj@...>
Signed-off-by: Eric W. Biederman <ebiederm@...>
---
 fs/sysfs/file.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index f5ea468..faa1a80 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -604,17 +604,9 @@ int sysfs_chmod_file(struct kobject *kobj, struct attribute *attr, mode_t mode)
  mutex_lock(&inode->i_mutex);
 
  newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
- newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
- newattrs.ia_ctime = current_fs_time(inode->i_sb);
+ newattrs.ia_valid = ATTR_MODE;
  rc = sysfs_setattr(victim, &newattrs);
 
- if (rc == 0) {
- fsnotify_change(victim, newattrs.ia_valid);
- mutex_lock(&sysfs_mutex);
- victim_sd->s_mode = newattrs.ia_mode;
- mutex_unlock(&sysfs_mutex);
- }
-
  mutex_unlock(&inode->i_mutex);
  out:
  dput(victim);
--
1.6.5.2.143.g8cc62

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@...>

The granularity of sysfs time when we keep it is 1 ns.  Which
when passed to timestamp_trunc results in a nop.  So remove
the unnecessary function call making sysfs_setattr slightly
easier to read.

Acked-by: Tejun Heo <tj@...>
Signed-off-by: Eric W. Biederman <ebiederm@...>
---
 fs/sysfs/inode.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index 8a08250..fed7a74 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -103,14 +103,11 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
  if (ia_valid & ATTR_GID)
  iattrs->ia_gid = iattr->ia_gid;
  if (ia_valid & ATTR_ATIME)
- iattrs->ia_atime = timespec_trunc(iattr->ia_atime,
- inode->i_sb->s_time_gran);
+ iattrs->ia_atime = iattr->ia_atime;
  if (ia_valid & ATTR_MTIME)
- iattrs->ia_mtime = timespec_trunc(iattr->ia_mtime,
- inode->i_sb->s_time_gran);
+ iattrs->ia_mtime = iattr->ia_mtime;
  if (ia_valid & ATTR_CTIME)
- iattrs->ia_ctime = timespec_trunc(iattr->ia_ctime,
- inode->i_sb->s_time_gran);
+ iattrs->ia_ctime = iattr->ia_ctime;
  if (ia_valid & ATTR_MODE) {
  umode_t mode = iattr->ia_mode;
 
--
1.6.5.2.143.g8cc62

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@...>

Cleanly separate the work that is specific to setting the
attributes of a sysfs_dirent from what is needed to update
the attributes of a vfs inode.

Additionally grab the sysfs_mutex to keep any nasties from
surprising us when updating the sysfs_dirent.

Acked-by: Tejun Heo <tj@...>
Signed-off-by: Eric W. Biederman <ebiederm@...>
---
 fs/sysfs/inode.c |   52 ++++++++++++++++++++++++++++++++--------------------
 fs/sysfs/sysfs.h |    1 +
 2 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index fed7a74..fccfb55 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -64,30 +64,15 @@ struct sysfs_inode_attrs *sysfs_init_inode_attrs(struct sysfs_dirent *sd)
 
  return attrs;
 }
-int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
+
+int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
 {
- struct inode * inode = dentry->d_inode;
- struct sysfs_dirent * sd = dentry->d_fsdata;
  struct sysfs_inode_attrs *sd_attrs;
  struct iattr *iattrs;
  unsigned int ia_valid = iattr->ia_valid;
- int error;
-
- if (!sd)
- return -EINVAL;
 
  sd_attrs = sd->s_iattr;
 
- error = inode_change_ok(inode, iattr);
- if (error)
- return error;
-
- iattr->ia_valid &= ~ATTR_SIZE; /* ignore size changes */
-
- error = inode_setattr(inode, iattr);
- if (error)
- return error;
-
  if (!sd_attrs) {
  /* setting attributes for the first time, allocate now */
  sd_attrs = sysfs_init_inode_attrs(sd);
@@ -110,12 +95,39 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
  iattrs->ia_ctime = iattr->ia_ctime;
  if (ia_valid & ATTR_MODE) {
  umode_t mode = iattr->ia_mode;
-
- if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
- mode &= ~S_ISGID;
  iattrs->ia_mode = sd->s_mode = mode;
  }
  }
+ return 0;
+}
+
+int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
+{
+ struct inode * inode = dentry->d_inode;
+ struct sysfs_dirent * sd = dentry->d_fsdata;
+ int error;
+
+ if (!sd)
+ return -EINVAL;
+
+ error = inode_change_ok(inode, iattr);
+ if (error)
+ return error;
+
+ iattr->ia_valid &= ~ATTR_SIZE; /* ignore size changes */
+ if (iattr->ia_valid & ATTR_MODE) {
+ if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
+ iattr->ia_mode &= ~S_ISGID;
+ }
+
+ error = inode_setattr(inode, iattr);
+ if (error)
+ return error;
+
+ mutex_lock(&sysfs_mutex);
+ error = sysfs_sd_setattr(sd, iattr);
+ mutex_unlock(&sysfs_mutex);
+
  return error;
 }
 
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index af4c4e7..a96d967 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -155,6 +155,7 @@ static inline void __sysfs_put(struct sysfs_dirent *sd)
  */
 struct inode *sysfs_get_inode(struct sysfs_dirent *sd);
 void sysfs_delete_inode(struct inode *inode);
+int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr *iattr);
 int sysfs_setattr(struct dentry *dentry, struct iattr *iattr);
 int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
  size_t size, int flags);
--
1.6.5.2.143.g8cc62

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@...>

Currently sysfs updates the timestamps on the vfs directory
inode when we create or remove a directory entry but doesn't
update the cached copy on the sysfs_dirent, fix that oversight.

Acked-by: Tejun Heo <tj@...>
Signed-off-by: Eric W. Biederman <ebiederm@...>
---
 fs/sysfs/dir.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index b5e8499..fa37126 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -464,6 +464,8 @@ void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
  */
 int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 {
+ struct sysfs_inode_attrs *ps_iattr;
+
  if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
  return -EEXIST;
 
@@ -476,6 +478,13 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 
  sysfs_link_sibling(sd);
 
+ /* Update timestamps on the parent */
+ ps_iattr = acxt->parent_sd->s_iattr;
+ if (ps_iattr) {
+ struct iattr *ps_iattrs = &ps_iattr->ia_iattr;
+ ps_iattrs->ia_ctime = ps_iattrs->ia_mtime = CURRENT_TIME;
+ }
+
  return 0;
 }
 
@@ -554,10 +563,19 @@ int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
  */
 void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 {
+ struct sysfs_inode_attrs *ps_iattr;
+
  BUG_ON(sd->s_flags & SYSFS_FLAG_REMOVED);
 
  sysfs_unlink_sibling(sd);
 
+ /* Update timestamps on the parent */
+ ps_iattr = acxt->parent_sd->s_iattr;
+ if (ps_iattr) {
+ struct iattr *ps_iattrs = &ps_iattr->ia_iattr;
+ ps_iattrs->ia_ctime = ps_iattrs->ia_mtime = CURRENT_TIME;
+ }
+
  sd->s_flags |= SYSFS_FLAG_REMOVED;
  sd->s_sibling = acxt->removed;
  acxt->removed = sd;
--
1.6.5.2.143.g8cc62

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@...>

Lining up the functions in sysfs_symlink_inode_operations
follows the pattern in the rest of sysfs and makes things
slightly more readable.

Acked-by: Tejun Heo <tj@...>
Signed-off-by: Eric W. Biederman <ebiederm@...>
---
 fs/sysfs/symlink.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index c5081ad..1137418 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -210,10 +210,10 @@ static void sysfs_put_link(struct dentry *dentry, struct nameidata *nd, void *co
 }
 
 const struct inode_operations sysfs_symlink_inode_operations = {
- .setxattr = sysfs_setxattr,
- .readlink = generic_readlink,
- .follow_link = sysfs_follow_link,
- .put_link = sysfs_put_link,
+ .setxattr = sysfs_setxattr,
+ .readlink = generic_readlink,
+ .follow_link = sysfs_follow_link,
+ .put_link = sysfs_put_link,
 };
 
 
--
1.6.5.2.143.g8cc62

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@...>

With the implementation of sysfs_getattr and sysfs_permission
sysfs becomes able to lazily propogate inode attribute changes
from the sysfs_dirents to the vfs inodes.   This paves the way
for deleting significant chunks of now unnecessary code.

Acked-by: Tejun Heo <tj@...>
Signed-off-by: Eric W. Biederman <ebiederm@...>
---
 fs/sysfs/dir.c     |    2 +
 fs/sysfs/inode.c   |   64 ++++++++++++++++++++++++++++++++++++++-------------
 fs/sysfs/symlink.c |    3 ++
 fs/sysfs/sysfs.h   |    2 +
 4 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index fa37126..25d052a 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -800,7 +800,9 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry,
 
 const struct inode_operations sysfs_dir_inode_operations = {
  .lookup = sysfs_lookup,
+ .permission = sysfs_permission,
  .setattr = sysfs_setattr,
+ .getattr = sysfs_getattr,
  .setxattr = sysfs_setxattr,
 };
 
diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index fccfb55..2dcafe8 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -37,7 +37,9 @@ static struct backing_dev_info sysfs_backing_dev_info = {
 };
 
 static const struct inode_operations sysfs_inode_operations ={
+ .permission = sysfs_permission,
  .setattr = sysfs_setattr,
+ .getattr = sysfs_getattr,
  .setxattr = sysfs_setxattr,
 };
 
@@ -196,7 +198,6 @@ static inline void set_default_inode_attr(struct inode * inode, mode_t mode)
 
 static inline void set_inode_attr(struct inode * inode, struct iattr * iattr)
 {
- inode->i_mode = iattr->ia_mode;
  inode->i_uid = iattr->ia_uid;
  inode->i_gid = iattr->ia_gid;
  inode->i_atime = iattr->ia_atime;
@@ -227,38 +228,56 @@ static int sysfs_count_nlink(struct sysfs_dirent *sd)
  return nr + 2;
 }
 
+static void sysfs_refresh_inode(struct sysfs_dirent *sd, struct inode *inode)
+{
+ struct sysfs_inode_attrs *iattrs = sd->s_iattr;
+
+ inode->i_mode = sd->s_mode;
+ if (iattrs) {
+ /* sysfs_dirent has non-default attributes
+ * get them from persistent copy in sysfs_dirent
+ */
+ set_inode_attr(inode, &iattrs->ia_iattr);
+ security_inode_notifysecctx(inode,
+    iattrs->ia_secdata,
+    iattrs->ia_secdata_len);
+ }
+
+ if (sysfs_type(sd) == SYSFS_DIR)
+ inode->i_nlink = sysfs_count_nlink(sd);
+}
+
+int sysfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat)
+{
+ struct sysfs_dirent *sd = dentry->d_fsdata;
+ struct inode *inode = dentry->d_inode;
+
+ mutex_lock(&sysfs_mutex);
+ sysfs_refresh_inode(sd, inode);
+ mutex_unlock(&sysfs_mutex);
+
+ generic_fillattr(inode, stat);
+ return 0;
+}
+
 static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)
 {
  struct bin_attribute *bin_attr;
- struct sysfs_inode_attrs *iattrs;
 
  inode->i_private = sysfs_get(sd);
  inode->i_mapping->a_ops = &sysfs_aops;
  inode->i_mapping->backing_dev_info = &sysfs_backing_dev_info;
  inode->i_op = &sysfs_inode_operations;
- inode->i_ino = sd->s_ino;
  lockdep_set_class(&inode->i_mutex, &sysfs_inode_imutex_key);
 
- iattrs = sd->s_iattr;
- if (iattrs) {
- /* sysfs_dirent has non-default attributes
- * get them for the new inode from persistent copy
- * in sysfs_dirent
- */
- set_inode_attr(inode, &iattrs->ia_iattr);
- if (iattrs->ia_secdata)
- security_inode_notifysecctx(inode,
- iattrs->ia_secdata,
- iattrs->ia_secdata_len);
- } else
- set_default_inode_attr(inode, sd->s_mode);
+ set_default_inode_attr(inode, sd->s_mode);
+ sysfs_refresh_inode(sd, inode);
 
  /* initialize inode according to type */
  switch (sysfs_type(sd)) {
  case SYSFS_DIR:
  inode->i_op = &sysfs_dir_inode_operations;
  inode->i_fop = &sysfs_dir_operations;
- inode->i_nlink = sysfs_count_nlink(sd);
  break;
  case SYSFS_KOBJ_ATTR:
  inode->i_size = PAGE_SIZE;
@@ -341,3 +360,14 @@ int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name)
  else
  return -ENOENT;
 }
+
+int sysfs_permission(struct inode *inode, int mask)
+{
+ struct sysfs_dirent *sd = inode->i_private;
+
+ mutex_lock(&sysfs_mutex);
+ sysfs_refresh_inode(sd, inode);
+ mutex_unlock(&sysfs_mutex);
+
+ return generic_permission(inode, mask, NULL);
+}
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 1137418..c5eff49 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -214,6 +214,9 @@ const struct inode_operations sysfs_symlink_inode_operations = {
  .readlink = generic_readlink,
  .follow_link = sysfs_follow_link,
  .put_link = sysfs_put_link,
+ .setattr = sysfs_setattr,
+ .getattr = sysfs_getattr,
+ .permission = sysfs_permission,
 };
 
 
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index a96d967..12ccc07 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -156,7 +156,9 @@ static inline void __sysfs_put(struct sysfs_dirent *sd)
 struct inode *sysfs_get_inode(struct sysfs_dirent *sd);
 void sysfs_delete_inode(struct inode *inode);
 int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr *iattr);
+int sysfs_permission(struct inode *inode, int mask);
 int sysfs_setattr(struct dentry *dentry, struct iattr *iattr);
+int sysfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat);
 int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
  size_t size, int flags);
 int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name);
--
1.6.5.2.143.g8cc62

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@...>

Now that sysfs_getattr and sysfs_permission refresh the vfs
inode there is no need to immediatly push the mode change
into the vfs cache.  Reducing the amount of work needed and
simplifying the locking.

Acked-by: Tejun Heo <tj@...>
Signed-off-by: Eric W. Biederman <ebiederm@...>
---
 fs/sysfs/file.c |   31 ++++++++-----------------------
 1 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index faa1a80..dc30d9e 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -579,38 +579,23 @@ EXPORT_SYMBOL_GPL(sysfs_add_file_to_group);
  */
 int sysfs_chmod_file(struct kobject *kobj, struct attribute *attr, mode_t mode)
 {
- struct sysfs_dirent *victim_sd = NULL;
- struct dentry *victim = NULL;
- struct inode * inode;
+ struct sysfs_dirent *sd;
  struct iattr newattrs;
  int rc;
 
- rc = -ENOENT;
- victim_sd = sysfs_get_dirent(kobj->sd, attr->name);
- if (!victim_sd)
- goto out;
+ mutex_lock(&sysfs_mutex);
 
- mutex_lock(&sysfs_rename_mutex);
- victim = sysfs_get_dentry(victim_sd);
- mutex_unlock(&sysfs_rename_mutex);
- if (IS_ERR(victim)) {
- rc = PTR_ERR(victim);
- victim = NULL;
+ rc = -ENOENT;
+ sd = sysfs_find_dirent(kobj->sd, attr->name);
+ if (!sd)
  goto out;
- }
-
- inode = victim->d_inode;
 
- mutex_lock(&inode->i_mutex);
-
- newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
+ newattrs.ia_mode = (mode & S_IALLUGO) | (sd->s_mode & ~S_IALLUGO);
  newattrs.ia_valid = ATTR_MODE;
- rc = sysfs_setattr(victim, &newattrs);
+ rc = sysfs_sd_setattr(sd, &newattrs);
 
- mutex_unlock(&inode->i_mutex);
  out:
- dput(victim);
- sysfs_put(victim_sd);
+ mutex_unlock(&sysfs_mutex);
  return rc;
 }
 EXPORT_SYMBOL_GPL(sysfs_chmod_file);
--
1.6.5.2.143.g8cc62

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@...>

With lazy inode updates and dentry operations bringing everything
into sync on demand there is no longer any need to immediately
update the vfs or grab i_mutex to protect those updates as we
make changes to sysfs.

Signed-off-by: Eric W. Biederman <ebiederm@...>
---
 fs/sysfs/dir.c   |   91 ++---------------------------------------------------
 fs/sysfs/sysfs.h |    2 -
 2 files changed, 4 insertions(+), 89 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 25d052a..a05b027 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -386,12 +386,6 @@ struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type)
  return NULL;
 }
 
-static int sysfs_ilookup_test(struct inode *inode, void *arg)
-{
- struct sysfs_dirent *sd = arg;
- return inode->i_ino == sd->s_ino;
-}
-
 /**
  * sysfs_addrm_start - prepare for sysfs_dirent add/remove
  * @acxt: pointer to sysfs_addrm_cxt to be used
@@ -399,47 +393,20 @@ static int sysfs_ilookup_test(struct inode *inode, void *arg)
  *
  * This function is called when the caller is about to add or
  * remove sysfs_dirent under @parent_sd.  This function acquires
- * sysfs_mutex, grabs inode for @parent_sd if available and lock
- * i_mutex of it.  @acxt is used to keep and pass context to
+ * sysfs_mutex.  @acxt is used to keep and pass context to
  * other addrm functions.
  *
  * LOCKING:
  * Kernel thread context (may sleep).  sysfs_mutex is locked on
- * return.  i_mutex of parent inode is locked on return if
- * available.
+ * return.
  */
 void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
        struct sysfs_dirent *parent_sd)
 {
- struct inode *inode;
-
  memset(acxt, 0, sizeof(*acxt));
  acxt->parent_sd = parent_sd;
 
- /* Lookup parent inode.  inode initialization is protected by
- * sysfs_mutex, so inode existence can be determined by
- * looking up inode while holding sysfs_mutex.
- */
  mutex_lock(&sysfs_mutex);
-
- inode = ilookup5(sysfs_sb, parent_sd->s_ino, sysfs_ilookup_test,
- parent_sd);
- if (inode) {
- WARN_ON(inode->i_state & I_NEW);
-
- /* parent inode available */
- acxt->parent_inode = inode;
-
- /* sysfs_mutex is below i_mutex in lock hierarchy.
- * First, trylock i_mutex.  If fails, unlock
- * sysfs_mutex and lock them in order.
- */
- if (!mutex_trylock(&inode->i_mutex)) {
- mutex_unlock(&sysfs_mutex);
- mutex_lock(&inode->i_mutex);
- mutex_lock(&sysfs_mutex);
- }
- }
 }
 
 /**
@@ -471,11 +438,6 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 
  sd->s_parent = sysfs_get(acxt->parent_sd);
 
- if (sysfs_type(sd) == SYSFS_DIR && acxt->parent_inode)
- inc_nlink(acxt->parent_inode);
-
- acxt->cnt++;
-
  sysfs_link_sibling(sd);
 
  /* Update timestamps on the parent */
@@ -579,40 +541,6 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
  sd->s_flags |= SYSFS_FLAG_REMOVED;
  sd->s_sibling = acxt->removed;
  acxt->removed = sd;
-
- if (sysfs_type(sd) == SYSFS_DIR && acxt->parent_inode)
- drop_nlink(acxt->parent_inode);
-
- acxt->cnt++;
-}
-
-/**
- * sysfs_dec_nlink - Decrement link count for the specified sysfs_dirent
- * @sd: target sysfs_dirent
- *
- * Decrement nlink for @sd.  @sd must have been unlinked from its
- * parent on entry to this function such that it can't be looked
- * up anymore.
- */
-static void sysfs_dec_nlink(struct sysfs_dirent *sd)
-{
- struct inode *inode;
-
- inode = ilookup(sysfs_sb, sd->s_ino);
- if (!inode)
- return;
-
- /* adjust nlink and update timestamp */
- mutex_lock(&inode->i_mutex);
-
- inode->i_ctime = CURRENT_TIME;
- drop_nlink(inode);
- if (sysfs_type(sd) == SYSFS_DIR)
- drop_nlink(inode);
-
- mutex_unlock(&inode->i_mutex);
-
- iput(inode);
 }
 
 /**
@@ -621,25 +549,15 @@ static void sysfs_dec_nlink(struct sysfs_dirent *sd)
  *
  * Finish up sysfs_dirent add/remove.  Resources acquired by
  * sysfs_addrm_start() are released and removed sysfs_dirents are
- * cleaned up.  Timestamps on the parent inode are updated.
+ * cleaned up.
  *
  * LOCKING:
- * All mutexes acquired by sysfs_addrm_start() are released.
+ * sysfs_mutex is released.
  */
 void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
 {
  /* release resources acquired by sysfs_addrm_start() */
  mutex_unlock(&sysfs_mutex);
- if (acxt->parent_inode) {
- struct inode *inode = acxt->parent_inode;
-
- /* if added/removed, update timestamps on the parent */
- if (acxt->cnt)
- inode->i_ctime = inode->i_mtime = CURRENT_TIME;
-
- mutex_unlock(&inode->i_mutex);
- iput(inode);
- }
 
  /* kill removed sysfs_dirents */
  while (acxt->removed) {
@@ -648,7 +566,6 @@ void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
  acxt->removed = sd->s_sibling;
  sd->s_sibling = NULL;
 
- sysfs_dec_nlink(sd);
  sysfs_deactivate(sd);
  unmap_bin_file(sd);
  sysfs_put(sd);
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 12ccc07..90b3501 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -89,9 +89,7 @@ static inline unsigned int sysfs_type(struct sysfs_dirent *sd)
  */
 struct sysfs_addrm_cxt {
  struct sysfs_dirent *parent_sd;
- struct inode *parent_inode;
  struct sysfs_dirent *removed;
- int cnt;
 };
 
 /*
--
1.6.5.2.143.g8cc62

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@...>

By teaching sysfs_revalidate to hide a dentry for
a sysfs_dirent if the sysfs_dirent has been renamed,
and by teaching sysfs_lookup to return the original
dentry if the sysfs dirent has been renamed.  I can
show the results of renames correctly without having to
update the dcache during the directory rename.

This massively simplifies the rename logic allowing a lot
of weird sysfs special cases to be removed along with
a lot of now unnecesary helper code.

Acked-by: Tejun Heo <tj@...>
Signed-off-by: Eric W. Biederman <ebiederm@...>
---
 fs/namei.c            |   22 -------
 fs/sysfs/dir.c        |  158 ++++++++++---------------------------------------
 fs/sysfs/inode.c      |   12 ----
 fs/sysfs/sysfs.h      |    1 -
 include/linux/namei.h |    1 -
 5 files changed, 32 insertions(+), 162 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index d11f404..d3c190c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1279,28 +1279,6 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
  return __lookup_hash(&this, base, NULL);
 }
 
-/**
- * lookup_one_noperm - bad hack for sysfs
- * @name: pathname component to lookup
- * @base: base directory to lookup from
- *
- * This is a variant of lookup_one_len that doesn't perform any permission
- * checks.   It's a horrible hack to work around the braindead sysfs
- * architecture and should not be used anywhere else.
- *
- * DON'T USE THIS FUNCTION EVER, thanks.
- */
-struct dentry *lookup_one_noperm(const char *name, struct dentry *base)
-{
- int err;
- struct qstr this;
-
- err = __lookup_one_len(name, &this, base, strlen(name));
- if (err)
- return ERR_PTR(err);
- return __lookup_hash(&this, base, NULL);
-}
-
 int user_path_at(int dfd, const char __user *name, unsigned flags,
  struct path *path)
 {
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index a05b027..0b60212 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -25,7 +25,6 @@
 #include "sysfs.h"
 
 DEFINE_MUTEX(sysfs_mutex);
-DEFINE_MUTEX(sysfs_rename_mutex);
 DEFINE_SPINLOCK(sysfs_assoc_lock);
 
 static DEFINE_SPINLOCK(sysfs_ino_lock);
@@ -85,46 +84,6 @@ static void sysfs_unlink_sibling(struct sysfs_dirent *sd)
 }
 
 /**
- * sysfs_get_dentry - get dentry for the given sysfs_dirent
- * @sd: sysfs_dirent of interest
- *
- * Get dentry for @sd.  Dentry is looked up if currently not
- * present.  This function descends from the root looking up
- * dentry for each step.
- *
- * LOCKING:
- * mutex_lock(sysfs_rename_mutex)
- *
- * RETURNS:
- * Pointer to found dentry on success, ERR_PTR() value on error.
- */
-struct dentry *sysfs_get_dentry(struct sysfs_dirent *sd)
-{
- struct dentry *dentry = dget(sysfs_sb->s_root);
-
- while (dentry->d_fsdata != sd) {
- struct sysfs_dirent *cur;
- struct dentry *parent;
-
- /* find the first ancestor which hasn't been looked up */
- cur = sd;
- while (cur->s_parent != dentry->d_fsdata)
- cur = cur->s_parent;
-
- /* look it up */
- parent = dentry;
- mutex_lock(&parent->d_inode->i_mutex);
- dentry = lookup_one_noperm(cur->s_name, parent);
- mutex_unlock(&parent->d_inode->i_mutex);
- dput(parent);
-
- if (IS_ERR(dentry))
- break;
- }
- return dentry;
-}
-
-/**
  * sysfs_get_active - get an active reference to sysfs_dirent
  * @sd: sysfs_dirent to get an active reference to
  *
@@ -315,6 +274,14 @@ static int sysfs_dentry_revalidate(struct dentry *dentry, struct nameidata *nd)
  if (sd->s_flags & SYSFS_FLAG_REMOVED)
  goto out_bad;
 
+ /* The sysfs dirent has been moved? */
+ if (dentry->d_parent->d_fsdata != sd->s_parent)
+ goto out_bad;
+
+ /* The sysfs dirent has been renamed */
+ if (strcmp(dentry->d_name.name, sd->s_name) != 0)
+ goto out_bad;
+
  mutex_unlock(&sysfs_mutex);
 out_valid:
  return 1;
@@ -322,6 +289,12 @@ out_bad:
  /* Remove the dentry from the dcache hashes.
  * If this is a deleted dentry we use d_drop instead of d_delete
  * so sysfs doesn't need to cope with negative dentries.
+ *
+ * If this is a dentry that has simply been renamed we
+ * use d_drop to remove it from the dcache lookup on its
+ * old parent.  If this dentry persists later when a lookup
+ * is performed at its new name the dentry will be readded
+ * to the dcache hashes.
  */
  is_dir = (sysfs_type(sd) == SYSFS_DIR);
  mutex_unlock(&sysfs_mutex);
@@ -705,10 +678,15 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry,
  }
 
  /* instantiate and hash dentry */
- dentry->d_op = &sysfs_dentry_ops;
- dentry->d_fsdata = sysfs_get(sd);
- d_instantiate(dentry, inode);
- d_rehash(dentry);
+ ret = d_find_alias(inode);
+ if (!ret) {
+ dentry->d_op = &sysfs_dentry_ops;
+ dentry->d_fsdata = sysfs_get(sd);
+ d_add(dentry, inode);
+ } else {
+ d_move(ret, dentry);
+ iput(inode);
+ }
 
  out_unlock:
  mutex_unlock(&sysfs_mutex);
@@ -785,62 +763,32 @@ void sysfs_remove_dir(struct kobject * kobj)
 int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 {
  struct sysfs_dirent *sd = kobj->sd;
- struct dentry *parent = NULL;
- struct dentry *old_dentry = NULL, *new_dentry = NULL;
  const char *dup_name = NULL;
  int error;
 
- mutex_lock(&sysfs_rename_mutex);
+ mutex_lock(&sysfs_mutex);
 
  error = 0;
  if (strcmp(sd->s_name, new_name) == 0)
  goto out; /* nothing to rename */
 
- /* get the original dentry */
- old_dentry = sysfs_get_dentry(sd);
- if (IS_ERR(old_dentry)) {
- error = PTR_ERR(old_dentry);
- old_dentry = NULL;
- goto out;
- }
-
- parent = old_dentry->d_parent;
-
- /* lock parent and get dentry for new name */
- mutex_lock(&parent->d_inode->i_mutex);
- mutex_lock(&sysfs_mutex);
-
  error = -EEXIST;
  if (sysfs_find_dirent(sd->s_parent, new_name))
- goto out_unlock;
-
- error = -ENOMEM;
- new_dentry = d_alloc_name(parent, new_name);
- if (!new_dentry)
- goto out_unlock;
+ goto out;
 
  /* rename sysfs_dirent */
  error = -ENOMEM;
  new_name = dup_name = kstrdup(new_name, GFP_KERNEL);
  if (!new_name)
- goto out_unlock;
+ goto out;
 
  dup_name = sd->s_name;
  sd->s_name = new_name;
 
- /* rename */
- d_add(new_dentry, NULL);
- d_move(old_dentry, new_dentry);
-
  error = 0;
- out_unlock:
+ out:
  mutex_unlock(&sysfs_mutex);
- mutex_unlock(&parent->d_inode->i_mutex);
  kfree(dup_name);
- dput(old_dentry);
- dput(new_dentry);
- out:
- mutex_unlock(&sysfs_rename_mutex);
  return error;
 }
 
@@ -848,12 +796,11 @@ int sysfs_move_dir(struct kobject *kobj, struct kobject *new_parent_kobj)
 {
  struct sysfs_dirent *sd = kobj->sd;
  struct sysfs_dirent *new_parent_sd;
- struct dentry *old_parent, *new_parent = NULL;
- struct dentry *old_dentry = NULL, *new_dentry = NULL;
  int error;
 
- mutex_lock(&sysfs_rename_mutex);
  BUG_ON(!sd->s_parent);
+
+ mutex_lock(&sysfs_mutex);
  new_parent_sd = (new_parent_kobj && new_parent_kobj->sd) ?
  new_parent_kobj->sd : &sysfs_root;
 
@@ -861,61 +808,20 @@ int sysfs_move_dir(struct kobject *kobj, struct kobject *new_parent_kobj)
  if (sd->s_parent == new_parent_sd)
  goto out; /* nothing to move */
 
- /* get dentries */
- old_dentry = sysfs_get_dentry(sd);
- if (IS_ERR(old_dentry)) {
- error = PTR_ERR(old_dentry);
- old_dentry = NULL;
- goto out;
- }
- old_parent = old_dentry->d_parent;
-
- new_parent = sysfs_get_dentry(new_parent_sd);
- if (IS_ERR(new_parent)) {
- error = PTR_ERR(new_parent);
- new_parent = NULL;
- goto out;
- }
-
-again:
- mutex_lock(&old_parent->d_inode->i_mutex);
- if (!mutex_trylock(&new_parent->d_inode->i_mutex)) {
- mutex_unlock(&old_parent->d_inode->i_mutex);
- goto again;
- }
- mutex_lock(&sysfs_mutex);
-
  error = -EEXIST;
  if (sysfs_find_dirent(new_parent_sd, sd->s_name))
- goto out_unlock;
-
- error = -ENOMEM;
- new_dentry = d_alloc_name(new_parent, sd->s_name);
- if (!new_dentry)
- goto out_unlock;
-
- error = 0;
- d_add(new_dentry, NULL);
- d_move(old_dentry, new_dentry);
+ goto out;
 
  /* Remove from old parent's list and insert into new parent's list. */
  sysfs_unlink_sibling(sd);
  sysfs_get(new_parent_sd);
- drop_nlink(old_parent->d_inode);
  sysfs_put(sd->s_parent);
  sd->s_parent = new_parent_sd;
- inc_nlink(new_parent->d_inode);
  sysfs_link_sibling(sd);
 
- out_unlock:
+ error = 0;
+out:
  mutex_unlock(&sysfs_mutex);
- mutex_unlock(&new_parent->d_inode->i_mutex);
- mutex_unlock(&old_parent->d_inode->i_mutex);
- out:
- dput(new_parent);
- dput(old_dentry);
- dput(new_dentry);
- mutex_unlock(&sysfs_rename_mutex);
  return error;
 }
 
diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index 2dcafe8..082a3eb 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -205,17 +205,6 @@ static inline void set_inode_attr(struct inode * inode, struct iattr * iattr)
  inode->i_ctime = iattr->ia_ctime;
 }
 
-
-/*
- * sysfs has a different i_mutex lock order behavior for i_mutex than other
- * filesystems; sysfs i_mutex is called in many places with subsystem locks
- * held. At the same time, many of the VFS locking rules do not apply to
- * sysfs at all (cross directory rename for example). To untangle this mess
- * (which gives false positives in lockdep), we're giving sysfs inodes their
- * own class for i_mutex.
- */
-static struct lock_class_key sysfs_inode_imutex_key;
-
 static int sysfs_count_nlink(struct sysfs_dirent *sd)
 {
  struct sysfs_dirent *child;
@@ -268,7 +257,6 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)
  inode->i_mapping->a_ops = &sysfs_aops;
  inode->i_mapping->backing_dev_info = &sysfs_backing_dev_info;
  inode->i_op = &sysfs_inode_operations;
- lockdep_set_class(&inode->i_mutex, &sysfs_inode_imutex_key);
 
  set_default_inode_attr(inode, sd->s_mode);
  sysfs_refresh_inode(sd, inode);
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 90b3501..98a15bf 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -103,7 +103,6 @@ extern struct kmem_cache *sysfs_dir_cachep;
  * dir.c
  */
 extern struct mutex sysfs_mutex;
-extern struct mutex sysfs_rename_mutex;
 extern spinlock_t sysfs_assoc_lock;
 
 extern const struct file_operations sysfs_dir_operations;
diff --git a/include/linux/namei.h b/include/linux/namei.h
index ec0f607..0289467 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -76,7 +76,6 @@ extern struct file *nameidata_to_filp(struct nameidata *nd, int flags);
 extern void release_open_intent(struct nameidata *);
 
 extern struct dentry *lookup_one_len(const char *, struct dentry *, int);
-extern struct dentry *lookup_one_noperm(const char *, struct dentry *);
 
 extern int follow_down(struct path *);
 extern int follow_up(struct path *);
--
1.6.5.2.143.g8cc62

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@...>

These two functions do 90% of the same work and it doesn't significantly
obfuscate the function to allow both the parent dir and the name to change
at the same time.  So merge them together to simplify maintenance, and
increase testing.

Acked-by: Tejun Heo <tj@...>
Signed-off-by: Eric W. Biederman <ebiederm@...>
---
 fs/sysfs/dir.c   |   62 +++++++++++++++++++++++++----------------------------
 fs/sysfs/sysfs.h |    3 ++
 2 files changed, 32 insertions(+), 33 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 0b60212..e1a86d1 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -760,30 +760,42 @@ void sysfs_remove_dir(struct kobject * kobj)
  __sysfs_remove_dir(sd);
 }
 
-int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
+int sysfs_rename(struct sysfs_dirent *sd,
+ struct sysfs_dirent *new_parent_sd, const char *new_name)
 {
- struct sysfs_dirent *sd = kobj->sd;
  const char *dup_name = NULL;
  int error;
 
  mutex_lock(&sysfs_mutex);
 
  error = 0;
- if (strcmp(sd->s_name, new_name) == 0)
+ if ((sd->s_parent == new_parent_sd) &&
+    (strcmp(sd->s_name, new_name) == 0))
  goto out; /* nothing to rename */
 
  error = -EEXIST;
- if (sysfs_find_dirent(sd->s_parent, new_name))
+ if (sysfs_find_dirent(new_parent_sd, new_name))
  goto out;
 
  /* rename sysfs_dirent */
- error = -ENOMEM;
- new_name = dup_name = kstrdup(new_name, GFP_KERNEL);
- if (!new_name)
- goto out;
+ if (strcmp(sd->s_name, new_name) != 0) {
+ error = -ENOMEM;
+ new_name = dup_name = kstrdup(new_name, GFP_KERNEL);
+ if (!new_name)
+ goto out;
+
+ dup_name = sd->s_name;
+ sd->s_name = new_name;
+ }
 
- dup_name = sd->s_name;
- sd->s_name = new_name;
+ /* Remove from old parent's list and insert into new parent's list. */
+ if (sd->s_parent != new_parent_sd) {
+ sysfs_unlink_sibling(sd);
+ sysfs_get(new_parent_sd);
+ sysfs_put(sd->s_parent);
+ sd->s_parent = new_parent_sd;
+ sysfs_link_sibling(sd);
+ }
 
  error = 0;
  out:
@@ -792,37 +804,21 @@ int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
  return error;
 }
 
+int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
+{
+ return sysfs_rename(kobj->sd, kobj->sd->s_parent, new_name);
+}
+
 int sysfs_move_dir(struct kobject *kobj, struct kobject *new_parent_kobj)
 {
  struct sysfs_dirent *sd = kobj->sd;
  struct sysfs_dirent *new_parent_sd;
- int error;
 
  BUG_ON(!sd->s_parent);
-
- mutex_lock(&sysfs_mutex);
- new_parent_sd = (new_parent_kobj && new_parent_kobj->sd) ?
+ new_parent_sd = new_parent_kobj && new_parent_kobj->sd ?
  new_parent_kobj->sd : &sysfs_root;
 
- error = 0;
- if (sd->s_parent == new_parent_sd)
- goto out; /* nothing to move */
-
- error = -EEXIST;
- if (sysfs_find_dirent(new_parent_sd, sd->s_name))
- goto out;
-
- /* Remove from old parent's list and insert into new parent's list. */
- sysfs_unlink_sibling(sd);
- sysfs_get(new_parent_sd);
- sysfs_put(sd->s_parent);
- sd->s_parent = new_parent_sd;
- sysfs_link_sibling(sd);
-
- error = 0;
-out:
- mutex_unlock(&sysfs_mutex);
- return error;
+ return sysfs_rename(sd, new_parent_sd, sd->s_name);
 }
 
 /* Relationship between s_mode and the DT_xxx types */
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 98a15bf..ca52e7b 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -130,6 +130,9 @@ int sysfs_create_subdir(struct kobject *kobj, const char *name,
  struct sysfs_dirent **p_sd);
 void sysfs_remove_subdir(struct sysfs_dirent *sd);
 
+int sysfs_rename(struct sysfs_dirent *sd,
+ struct sysfs_dirent *new_parent_sd, const char *new_name);
+
 static inline struct sysfs_dirent *__sysfs_get(struct sysfs_dirent *sd)
 {
  if (sd) {
--
1.6.5.2.143.g8cc62

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm@...):

> From: Eric W. Biederman <ebiederm@...>
>
> The sysfs_mutex is required to ensure updates are and will remain
> atomic with respect to other inode iattr updates, that do not happen
> through the filesystem.
>
> Signed-off-by: Eric W. Biederman <ebiederm@...>
> ---
>  fs/sysfs/inode.c |   41 +++++++++++++++++++++++++++++------------
>  1 files changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
> index e28cecf..8a08250 100644
> --- a/fs/sysfs/inode.c
> +++ b/fs/sysfs/inode.c
> @@ -122,23 +122,39 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
>   return error;
>  }
>
> +static int sysfs_sd_setsecdata(struct sysfs_dirent *sd, void **secdata, u32 *secdata_len)
> +{
> + struct sysfs_inode_attrs *iattrs;
> + void *old_secdata;
> + size_t old_secdata_len;
> +
> + iattrs = sd->s_iattr;
> + if (!iattrs)
> + iattrs = sysfs_init_inode_attrs(sd);
> + if (!iattrs)
> + return -ENOMEM;
> +
> + old_secdata = iattrs->ia_secdata;
> + old_secdata_len = iattrs->ia_secdata_len;
> +
> + iattrs->ia_secdata = *secdata;
> + iattrs->ia_secdata_len = *secdata_len;
> +
> + *secdata = old_secdata;
> + *secdata_len = old_secdata_len;
> + return 0;
> +}
> +
>  int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
>   size_t size, int flags)
>  {
>   struct sysfs_dirent *sd = dentry->d_fsdata;
> - struct sysfs_inode_attrs *iattrs;
>   void *secdata;
>   int error;
>   u32 secdata_len = 0;
>
>   if (!sd)
>   return -EINVAL;
> - if (!sd->s_iattr)
> - sd->s_iattr = sysfs_init_inode_attrs(sd);
> - if (!sd->s_iattr)
> - return -ENOMEM;
> -
> - iattrs = sd->s_iattr;
>
>   if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
>   const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> @@ -150,12 +166,13 @@ int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
>   &secdata, &secdata_len);
>   if (error)
>   goto out;
> - if (iattrs->ia_secdata)
> - security_release_secctx(iattrs->ia_secdata,
> - iattrs->ia_secdata_len);
> - iattrs->ia_secdata = secdata;
> - iattrs->ia_secdata_len = secdata_len;
>
> + mutex_lock(&sysfs_mutex);
> + error = sysfs_sd_setsecdata(sd, &secdata, &secdata_len);

You're ignoring the potential -ENOMEM return value here?

Worse, if -ENOMEM, then secdata was never set, so you call
security_release_secctx() on a random value left on the stack...

> + mutex_unlock(&sysfs_mutex);
> +
> + if (secdata)
> + security_release_secctx(secdata, secdata_len);
>   } else
>   return -EINVAL;
>  out:
> --
> 1.6.5.2.143.g8cc62
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@...
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

"Serge E. Hallyn" <serue@...> writes:

> Quoting Eric W. Biederman (ebiederm@...):
>> From: Eric W. Biederman <ebiederm@...>
>>
>> The sysfs_mutex is required to ensure updates are and will remain
>> atomic with respect to other inode iattr updates, that do not happen
>> through the filesystem.
>>
>> Signed-off-by: Eric W. Biederman <ebiederm@...>
>> ---
>>  fs/sysfs/inode.c |   41 +++++++++++++++++++++++++++++------------
>>  1 files changed, 29 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
>> index e28cecf..8a08250 100644
>> --- a/fs/sysfs/inode.c
>> +++ b/fs/sysfs/inode.c
>> @@ -122,23 +122,39 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
>>   return error;
>>  }
>>
>> +static int sysfs_sd_setsecdata(struct sysfs_dirent *sd, void **secdata, u32 *secdata_len)
>> +{
>> + struct sysfs_inode_attrs *iattrs;
>> + void *old_secdata;
>> + size_t old_secdata_len;
>> +
>> + iattrs = sd->s_iattr;
>> + if (!iattrs)
>> + iattrs = sysfs_init_inode_attrs(sd);
>> + if (!iattrs)
>> + return -ENOMEM;
>> +
>> + old_secdata = iattrs->ia_secdata;
>> + old_secdata_len = iattrs->ia_secdata_len;
>> +
>> + iattrs->ia_secdata = *secdata;
>> + iattrs->ia_secdata_len = *secdata_len;
>> +
>> + *secdata = old_secdata;
>> + *secdata_len = old_secdata_len;
>> + return 0;
>> +}
>> +
>>  int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
>>   size_t size, int flags)
>>  {
>>   struct sysfs_dirent *sd = dentry->d_fsdata;
>> - struct sysfs_inode_attrs *iattrs;
>>   void *secdata;
>>   int error;
>>   u32 secdata_len = 0;
>>
>>   if (!sd)
>>   return -EINVAL;
>> - if (!sd->s_iattr)
>> - sd->s_iattr = sysfs_init_inode_attrs(sd);
>> - if (!sd->s_iattr)
>> - return -ENOMEM;
>> -
>> - iattrs = sd->s_iattr;
>>
>>   if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
>>   const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
>> @@ -150,12 +166,13 @@ int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
>>   &secdata, &secdata_len);
>>   if (error)
>>   goto out;
>> - if (iattrs->ia_secdata)
>> - security_release_secctx(iattrs->ia_secdata,
>> - iattrs->ia_secdata_len);
>> - iattrs->ia_secdata = secdata;
>> - iattrs->ia_secdata_len = secdata_len;
>>
>> + mutex_lock(&sysfs_mutex);
>> + error = sysfs_sd_setsecdata(sd, &secdata, &secdata_len);
>
> You're ignoring the potential -ENOMEM return value here?
>
> Worse, if -ENOMEM, then secdata was never set, so you call
> security_release_secctx() on a random value left on the stack...

No.  It is more elegant than that.
If sysfs_sd_setsecdata fails secdata holds the freshly allocated secdata value.
If sysfs_sd_setsecdata succeeds secdata holds the old value of secdata.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm@...):

> "Serge E. Hallyn" <serue@...> writes:
>
> > Quoting Eric W. Biederman (ebiederm@...):
> >> From: Eric W. Biederman <ebiederm@...>
> >>
> >> The sysfs_mutex is required to ensure updates are and will remain
> >> atomic with respect to other inode iattr updates, that do not happen
> >> through the filesystem.
> >>
> >> Signed-off-by: Eric W. Biederman <ebiederm@...>
> >> ---
> >>  fs/sysfs/inode.c |   41 +++++++++++++++++++++++++++++------------
> >>  1 files changed, 29 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
> >> index e28cecf..8a08250 100644
> >> --- a/fs/sysfs/inode.c
> >> +++ b/fs/sysfs/inode.c
> >> @@ -122,23 +122,39 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
> >>   return error;
> >>  }
> >>
> >> +static int sysfs_sd_setsecdata(struct sysfs_dirent *sd, void **secdata, u32 *secdata_len)
> >> +{
> >> + struct sysfs_inode_attrs *iattrs;
> >> + void *old_secdata;
> >> + size_t old_secdata_len;
> >> +
> >> + iattrs = sd->s_iattr;
> >> + if (!iattrs)
> >> + iattrs = sysfs_init_inode_attrs(sd);
> >> + if (!iattrs)
> >> + return -ENOMEM;
> >> +
> >> + old_secdata = iattrs->ia_secdata;
> >> + old_secdata_len = iattrs->ia_secdata_len;
> >> +
> >> + iattrs->ia_secdata = *secdata;
> >> + iattrs->ia_secdata_len = *secdata_len;
> >> +
> >> + *secdata = old_secdata;
> >> + *secdata_len = old_secdata_len;
> >> + return 0;
> >> +}
> >> +
> >>  int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
> >>   size_t size, int flags)
> >>  {
> >>   struct sysfs_dirent *sd = dentry->d_fsdata;
> >> - struct sysfs_inode_attrs *iattrs;
> >>   void *secdata;
> >>   int error;
> >>   u32 secdata_len = 0;
> >>
> >>   if (!sd)
> >>   return -EINVAL;
> >> - if (!sd->s_iattr)
> >> - sd->s_iattr = sysfs_init_inode_attrs(sd);
> >> - if (!sd->s_iattr)
> >> - return -ENOMEM;
> >> -
> >> - iattrs = sd->s_iattr;
> >>
> >>   if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
> >>   const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> >> @@ -150,12 +166,13 @@ int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
> >>   &secdata, &secdata_len);
> >>   if (error)
> >>   goto out;
> >> - if (iattrs->ia_secdata)
> >> - security_release_secctx(iattrs->ia_secdata,
> >> - iattrs->ia_secdata_len);
> >> - iattrs->ia_secdata = secdata;
> >> - iattrs->ia_secdata_len = secdata_len;
> >>
> >> + mutex_lock(&sysfs_mutex);
> >> + error = sysfs_sd_setsecdata(sd, &secdata, &secdata_len);
> >
> > You're ignoring the potential -ENOMEM return value here?
> >
> > Worse, if -ENOMEM, then secdata was never set, so you call
> > security_release_secctx() on a random value left on the stack...
>
> No.  It is more elegant than that.
> If sysfs_sd_setsecdata fails secdata holds the freshly allocated secdata value.
> If sysfs_sd_setsecdata succeeds secdata holds the old value of secdata.

Gah, sorry.

Acked-by: Serge Hallyn <serue@...>

(as an aside, I can't see any reason to not just return if strncmp(name,
XATTR_SECURITY_PREFIX,) above to avoid a level of indent and needless gotos)

thanks,
-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Eric W. Biederman]

"Serge E. Hallyn" <serue@...> writes:
>
> Gah, sorry.

No prob.

> Acked-by: Serge Hallyn <serue@...>
>
> (as an aside, I can't see any reason to not just return if strncmp(name,
> XATTR_SECURITY_PREFIX,) above to avoid a level of indent and needless gotos)

Sounds like it is worth a patch.  For now I'm concentrating on getting
this patchset merged, and extraneous cleanups have a way of dragging
the process out beyond what time I have to give.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm@...):
> From: Eric W. Biederman <ebiederm@...>
>
> Calling d_drop unconditionally when a sysfs_dirent is deleted has
> the potential to leak mounts, so instead implement dentry delete
> and revalidate operations that cause sysfs dentries to be removed
> at the appropriate time.
>
> Acked-by: Tejun Heo <tj@...>
> Signed-off-by: Eric W. Biederman <ebiederm@...>

I like where it's heading (and spent some time staring at the
related code, looks kosher).

Acked-by: Serge Hallyn <serue@...>

> ---
>  fs/sysfs/dir.c |   73 +++++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 46 insertions(+), 27 deletions(-)
>
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index 130dfc3..b5e8499 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -298,6 +298,46 @@ void release_sysfs_dirent(struct sysfs_dirent * sd)
>   goto repeat;
>  }
>
> +static int sysfs_dentry_delete(struct dentry *dentry)
> +{
> + struct sysfs_dirent *sd = dentry->d_fsdata;
> + return !!(sd->s_flags & SYSFS_FLAG_REMOVED);
> +}
> +
> +static int sysfs_dentry_revalidate(struct dentry *dentry, struct nameidata *nd)
> +{
> + struct sysfs_dirent *sd = dentry->d_fsdata;
> + int is_dir;
> +
> + mutex_lock(&sysfs_mutex);
> +
> + /* The sysfs dirent has been deleted */
> + if (sd->s_flags & SYSFS_FLAG_REMOVED)
> + goto out_bad;
> +
> + mutex_unlock(&sysfs_mutex);
> +out_valid:
> + return 1;
> +out_bad:
> + /* Remove the dentry from the dcache hashes.
> + * If this is a deleted dentry we use d_drop instead of d_delete
> + * so sysfs doesn't need to cope with negative dentries.
> + */
> + is_dir = (sysfs_type(sd) == SYSFS_DIR);
> + mutex_unlock(&sysfs_mutex);
> + if (is_dir) {
> + /* If we have submounts we must allow the vfs caches
> + * to lie about the state of the filesystem to prevent
> + * leaks and other nasty things.
> + */
> + if (have_submounts(dentry))
> + goto out_valid;
> + shrink_dcache_parent(dentry);
> + }
> + d_drop(dentry);
> + return 0;
> +}
> +
>  static void sysfs_dentry_iput(struct dentry * dentry, struct inode * inode)
>  {
>   struct sysfs_dirent * sd = dentry->d_fsdata;
> @@ -307,6 +347,8 @@ static void sysfs_dentry_iput(struct dentry * dentry, struct inode * inode)
>  }
>
>  static const struct dentry_operations sysfs_dentry_ops = {
> + .d_revalidate = sysfs_dentry_revalidate,
> + .d_delete = sysfs_dentry_delete,
>   .d_iput = sysfs_dentry_iput,
>  };
>
> @@ -527,44 +569,21 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
>  }
>
>  /**
> - * sysfs_drop_dentry - drop dentry for the specified sysfs_dirent
> + * sysfs_dec_nlink - Decrement link count for the specified sysfs_dirent
>   * @sd: target sysfs_dirent
>   *
> - * Drop dentry for @sd.  @sd must have been unlinked from its
> + * Decrement nlink for @sd.  @sd must have been unlinked from its
>   * parent on entry to this function such that it can't be looked
>   * up anymore.
>   */
> -static void sysfs_drop_dentry(struct sysfs_dirent *sd)
> +static void sysfs_dec_nlink(struct sysfs_dirent *sd)
>  {
>   struct inode *inode;
> - struct dentry *dentry;
>
>   inode = ilookup(sysfs_sb, sd->s_ino);
>   if (!inode)
>   return;
>
> - /* Drop any existing dentries associated with sd.
> - *
> - * For the dentry to be properly freed we need to grab a
> - * reference to the dentry under the dcache lock,  unhash it,
> - * and then put it.  The playing with the dentry count allows
> - * dput to immediately free the dentry  if it is not in use.
> - */
> -repeat:
> - spin_lock(&dcache_lock);
> - list_for_each_entry(dentry, &inode->i_dentry, d_alias) {
> - if (d_unhashed(dentry))
> - continue;
> - dget_locked(dentry);
> - spin_lock(&dentry->d_lock);
> - __d_drop(dentry);
> - spin_unlock(&dentry->d_lock);
> - spin_unlock(&dcache_lock);
> - dput(dentry);
> - goto repeat;
> - }
> - spin_unlock(&dcache_lock);
> -
>   /* adjust nlink and update timestamp */
>   mutex_lock(&inode->i_mutex);
>
> @@ -611,7 +630,7 @@ void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
>   acxt->removed = sd->s_sibling;
>   sd->s_sibling = NULL;
>
> - sysfs_drop_dentry(sd);
> + sysfs_dec_nlink(sd);
>   sysfs_deactivate(sd);
>   unmap_bin_file(sd);
>   sysfs_put(sd);
> --
> 1.6.5.2.143.g8cc62
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@...
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm@...):
> From: Eric W. Biederman <ebiederm@...>
>
> Currently every caller of sysfs_chmod_file happens at either
> file creation time to set a non-default mode or in response
> to a specific user requested space change in policy.  Making
> timestamps of when the chmod happens and notification of
> a file changing mode uninteresting.

But these changes can occur by togging values in sysfs files
(i.e. f71805f.c), right?  Is this (specifically not doing inotify)
definately uncontroversial?

I can't exactly picture an admin sitting there watching
nautilus for a sysfs file to become writeable, but could
imagine some site's automation getting hung...  Or am I way
off base?

> Remove the unnecessary time stamp and filesystem change
> notification, and removes the last of the explicit inotify
> and donitfy support from sysfs.
>
> Acked-by: Tejun Heo <tj@...>
> Signed-off-by: Eric W. Biederman <ebiederm@...>
> ---
>  fs/sysfs/file.c |   10 +---------
>  1 files changed, 1 insertions(+), 9 deletions(-)
>
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index f5ea468..faa1a80 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -604,17 +604,9 @@ int sysfs_chmod_file(struct kobject *kobj, struct attribute *attr, mode_t mode)
>   mutex_lock(&inode->i_mutex);
>
>   newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
> - newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
> - newattrs.ia_ctime = current_fs_time(inode->i_sb);
> + newattrs.ia_valid = ATTR_MODE;
>   rc = sysfs_setattr(victim, &newattrs);
>
> - if (rc == 0) {
> - fsnotify_change(victim, newattrs.ia_valid);
> - mutex_lock(&sysfs_mutex);
> - victim_sd->s_mode = newattrs.ia_mode;
> - mutex_unlock(&sysfs_mutex);
> - }
> -
>   mutex_unlock(&inode->i_mutex);
>   out:
>   dput(victim);
> --
> 1.6.5.2.143.g8cc62
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@...
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/