* [PATCH] [ConfigFS]: Add struct configfs_item_operations->check_link() @ 2009-04-03 8:25 Nicholas A. Bellinger 2009-04-03 9:51 ` Louis Rilling 0 siblings, 1 reply; 3+ messages in thread From: Nicholas A. Bellinger @ 2009-04-03 8:25 UTC (permalink / raw) To: LKML, Linux-fsdevel, linux-scsi; +Cc: Joel Becker, Andrew Morton Hi Joel and co, This patch adds struct configfs_item_operations->check_link() and changes fs/configfs/symlink.c:configfs_unlink() so that when (*check_link) is present, an ConfigFS unlink will fail, based upon input by said symlinked struct config_item *parent_item. If a non zero return is returned from (*check_link), said non zero value is expected to use include/asm-generic/errno* values, and the failure is returned to userspace via the unlink(2) system call. Please consider this patch for v2.6.30. It requires no changes to existing consumers of ConfigFS like fs/ocfs2, and I have tested it with running LIO-Target v3.0 code. Many thanks for your most valuable of time, --nab Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org> --- fs/configfs/symlink.c | 13 +++++++++++++ include/linux/configfs.h | 1 + 2 files changed, 14 insertions(+), 0 deletions(-) diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c index 932a92b..a5dede6 100644 --- a/fs/configfs/symlink.c +++ b/fs/configfs/symlink.c @@ -202,6 +202,19 @@ int configfs_unlink(struct inode *dir, struct dentry *dentry) parent_item = configfs_get_config_item(dentry->d_parent); type = parent_item->ci_type; + /* + * See if the underlying struct config_item has dependent + * symlinks, and should return -EACCES here. + */ + if (type && type->ct_item_ops && + type->ct_item_ops->check_link) { + ret = type->ct_item_ops->check_link(parent_item); + if (ret != 0) { + config_item_put(parent_item); + goto out; + } + } + spin_lock(&configfs_dirent_lock); list_del_init(&sd->s_sibling); spin_unlock(&configfs_dirent_lock); diff --git a/include/linux/configfs.h b/include/linux/configfs.h index 7f62777..b026f16 100644 --- a/include/linux/configfs.h +++ b/include/linux/configfs.h @@ -226,6 +226,7 @@ struct configfs_item_operations { ssize_t (*show_attribute)(struct config_item *, struct configfs_attribute *,char *); ssize_t (*store_attribute)(struct config_item *,struct configfs_attribute *,const char *, size_t); int (*allow_link)(struct config_item *src, struct config_item *target); + int (*check_link)(struct config_item *src); int (*drop_link)(struct config_item *src, struct config_item *target); }; -- 1.5.4.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] [ConfigFS]: Add struct configfs_item_operations->check_link() 2009-04-03 8:25 [PATCH] [ConfigFS]: Add struct configfs_item_operations->check_link() Nicholas A. Bellinger @ 2009-04-03 9:51 ` Louis Rilling 2009-04-03 10:47 ` Nicholas A. Bellinger 0 siblings, 1 reply; 3+ messages in thread From: Louis Rilling @ 2009-04-03 9:51 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: LKML, Linux-fsdevel, linux-scsi, Joel Becker, Andrew Morton [-- Attachment #1: Type: text/plain, Size: 3386 bytes --] Hi, On 03/04/09 1:25 -0700, Nicholas A. Bellinger wrote: > Hi Joel and co, > > This patch adds struct configfs_item_operations->check_link() and > changes fs/configfs/symlink.c:configfs_unlink() so that > when (*check_link) is present, an ConfigFS unlink will fail, based upon > input by said symlinked struct config_item *parent_item. > > If a non zero return is returned from (*check_link), said non zero value is > expected to use include/asm-generic/errno* values, and the failure is returned > to userspace via the unlink(2) system call. > > Please consider this patch for v2.6.30. It requires no changes to existing consumers > of ConfigFS like fs/ocfs2, and I have tested it with running LIO-Target v3.0 code. > > Many thanks for your most valuable of time, I can't judge the actual need for that since I don't really know your usecase (I've seen the second patch). However check_link() without target_item as parameter looks a bit restrictive for no valuable reason. See inline for a concern about the error returned. Other than that, the patch looks ok. Louis > > --nab > > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org> > --- > fs/configfs/symlink.c | 13 +++++++++++++ > include/linux/configfs.h | 1 + > 2 files changed, 14 insertions(+), 0 deletions(-) > > diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c > index 932a92b..a5dede6 100644 > --- a/fs/configfs/symlink.c > +++ b/fs/configfs/symlink.c > @@ -202,6 +202,19 @@ int configfs_unlink(struct inode *dir, struct dentry *dentry) > parent_item = configfs_get_config_item(dentry->d_parent); > type = parent_item->ci_type; > > + /* > + * See if the underlying struct config_item has dependent > + * symlinks, and should return -EACCES here. > + */ I think that -EPERM is more natural than -EACCES. check_link() actually checks that the operation is permitted. > + if (type && type->ct_item_ops && > + type->ct_item_ops->check_link) { > + ret = type->ct_item_ops->check_link(parent_item); > + if (ret != 0) { > + config_item_put(parent_item); > + goto out; > + } > + } > + > spin_lock(&configfs_dirent_lock); > list_del_init(&sd->s_sibling); > spin_unlock(&configfs_dirent_lock); > diff --git a/include/linux/configfs.h b/include/linux/configfs.h > index 7f62777..b026f16 100644 > --- a/include/linux/configfs.h > +++ b/include/linux/configfs.h > @@ -226,6 +226,7 @@ struct configfs_item_operations { > ssize_t (*show_attribute)(struct config_item *, struct configfs_attribute *,char *); > ssize_t (*store_attribute)(struct config_item *,struct configfs_attribute *,const char *, size_t); > int (*allow_link)(struct config_item *src, struct config_item *target); > + int (*check_link)(struct config_item *src); > int (*drop_link)(struct config_item *src, struct config_item *target); > }; > > -- > 1.5.4.1 > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Dr Louis Rilling Kerlabs Skype: louis.rilling Batiment Germanium Phone: (+33|0) 6 80 89 08 23 80 avenue des Buttes de Coesmes http://www.kerlabs.com/ 35700 Rennes [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] [ConfigFS]: Add struct configfs_item_operations->check_link() 2009-04-03 9:51 ` Louis Rilling @ 2009-04-03 10:47 ` Nicholas A. Bellinger 0 siblings, 0 replies; 3+ messages in thread From: Nicholas A. Bellinger @ 2009-04-03 10:47 UTC (permalink / raw) To: Louis.Rilling; +Cc: LKML, Linux-fsdevel, linux-scsi, Joel Becker, Andrew Morton On Fri, 2009-04-03 at 11:51 +0200, Louis Rilling wrote: > Hi, > > On 03/04/09 1:25 -0700, Nicholas A. Bellinger wrote: > > Hi Joel and co, > > > > This patch adds struct configfs_item_operations->check_link() and > > changes fs/configfs/symlink.c:configfs_unlink() so that > > when (*check_link) is present, an ConfigFS unlink will fail, based upon > > input by said symlinked struct config_item *parent_item. > > > > If a non zero return is returned from (*check_link), said non zero value is > > expected to use include/asm-generic/errno* values, and the failure is returned > > to userspace via the unlink(2) system call. > > > > Please consider this patch for v2.6.30. It requires no changes to existing consumers > > of ConfigFS like fs/ocfs2, and I have tested it with running LIO-Target v3.0 code. > > > > Many thanks for your most valuable of time, > > I can't judge the actual need for that since I don't really know your usecase > (I've seen the second patch). However check_link() without target_item as > parameter looks a bit restrictive for no valuable reason. > > See inline for a concern about the error returned. > Other than that, the patch looks ok. > > Louis > > > > > --nab > > > > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org> > > --- > > fs/configfs/symlink.c | 13 +++++++++++++ > > include/linux/configfs.h | 1 + > > 2 files changed, 14 insertions(+), 0 deletions(-) > > > > diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c > > index 932a92b..a5dede6 100644 > > --- a/fs/configfs/symlink.c > > +++ b/fs/configfs/symlink.c > > @@ -202,6 +202,19 @@ int configfs_unlink(struct inode *dir, struct dentry *dentry) > > parent_item = configfs_get_config_item(dentry->d_parent); > > type = parent_item->ci_type; > > > > + /* > > + * See if the underlying struct config_item has dependent > > + * symlinks, and should return -EACCES here. > > + */ > > I think that -EPERM is more natural than -EACCES. check_link() actually checks > that the operation is permitted. > Greeings Louis, Makes sense to me. :-) Here is the updated commit for lio-core-2.6.git to return the default -EPERM from (*check_link), regardless of the non-zero return from the calling struct configfs_item_opreations. It also adds the 2nd parameter of type struct config_item for underlying ConfigFS consumer usage. Thank you for your comments! Many thanks for your most valuable of time, --nab >From 60309f152155c1382b2340bc87af200720b864b9 Mon Sep 17 00:00:00 2001 From: Nicholas Bellinger <nab@linux-iscsi.org> Date: Fri, 3 Apr 2009 03:36:11 -0700 Subject: [PATCH] [ConfigFS]: Add (*check_link) parameter and use -EPERM This patch updates struct configfs_item_operations->check_link() to use the 2nd parameter "struct config_item *target". It also updates the fs/configfs/symlink.c:configfs_unlink() caller for (*check_link) to look for a strict non-zero return, and return the default -EPERM from a local scope variable in configfs_unlink(). Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org> --- fs/configfs/symlink.c | 4 ++-- include/linux/configfs.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c index a5dede6..6c7db67 100644 --- a/fs/configfs/symlink.c +++ b/fs/configfs/symlink.c @@ -208,8 +208,8 @@ int configfs_unlink(struct inode *dir, struct dentry *dentry) */ if (type && type->ct_item_ops && type->ct_item_ops->check_link) { - ret = type->ct_item_ops->check_link(parent_item); - if (ret != 0) { + if (type->ct_item_ops->check_link(parent_item, + sl->sl_target) != 0) { config_item_put(parent_item); goto out; } diff --git a/include/linux/configfs.h b/include/linux/configfs.h index b026f16..fc072ce 100644 --- a/include/linux/configfs.h +++ b/include/linux/configfs.h @@ -226,7 +226,7 @@ struct configfs_item_operations { ssize_t (*show_attribute)(struct config_item *, struct configfs_attribute *,char *); ssize_t (*store_attribute)(struct config_item *,struct configfs_attribute *,const char *, size_t); int (*allow_link)(struct config_item *src, struct config_item *target); - int (*check_link)(struct config_item *src); + int (*check_link)(struct config_item *src, struct config_item *target); int (*drop_link)(struct config_item *src, struct config_item *target); }; -- 1.5.4.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2009-04-03 10:47 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-04-03 8:25 [PATCH] [ConfigFS]: Add struct configfs_item_operations->check_link() Nicholas A. Bellinger 2009-04-03 9:51 ` Louis Rilling 2009-04-03 10:47 ` Nicholas A. Bellinger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).