Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1535583 > unrolled thread
| Started by | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| First post | 2016-12-04 05:40 +0100 |
| Last post | 2016-12-05 15:50 +0100 |
| Articles | 5 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available [ver #3] Al Viro <viro@ZenIV.linux.org.uk> - 2016-12-04 05:40 +0100
Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available [ver #3] Al Viro <viro@ZenIV.linux.org.uk> - 2016-12-04 17:10 +0100
Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available [ver #3] David Howells <dhowells@redhat.com> - 2016-12-05 16:40 +0100
Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available [ver #3] Al Viro <viro@ZenIV.linux.org.uk> - 2016-12-04 18:40 +0100
Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available [ver #3] Miklos Szeredi <miklos@szeredi.hu> - 2016-12-05 15:50 +0100
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-12-04 05:40 +0100 |
| Subject | Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available [ver #3] |
| Message-ID | <sKwKJ-2uH-5@gated-at.bofh.it> |
On Wed, Nov 23, 2016 at 12:55:51AM +0000, David Howells wrote: > + * vfs_xgetattr - Get the enhanced basic attributes of a file > + * @path: The file of interest > + * @stat: Where to return the statistics > + * > + * Ask the filesystem for a file's attributes. The caller must have preset > + * stat->request_mask and stat->query_flags to indicate what they want. > + * > + * If the file is remote, the filesystem can be forced to update the attributes > + * from the backing store by passing AT_FORCE_ATTR_SYNC in query_flags or can > + * suppress the update by passing AT_NO_ATTR_SYNC. > + * > + * Bits must have been set in stat->request_mask to indicate which attributes > + * the caller wants retrieving. Any such attribute not requested may be > + * returned anyway, but the value may be approximate, and, if remote, may not > + * have been synchronised with the server. I understand wanting to avoid extra arguments, but you are asking for trouble with that sort of calling conventions. Verifying that all call chains have these fields initialized is bloody unpleasant and it *is* going to break, especially since the rules are "you need to initialize it for vfs_xgetattr(), but not for vfs_getattr()" - the names are similar enough for confusion, and that's not the only such pair.
[toc] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-12-04 17:10 +0100 |
| Message-ID | <sKHwt-WH-7@gated-at.bofh.it> |
| In reply to | #1535583 |
On Sun, Dec 04, 2016 at 04:38:05AM +0000, Al Viro wrote: > I understand wanting to avoid extra arguments, but you are asking for trouble > with that sort of calling conventions. Verifying that all call chains have > these fields initialized is bloody unpleasant and it *is* going to break, > especially since the rules are "you need to initialize it for vfs_xgetattr(), > but not for vfs_getattr()" - the names are similar enough for confusion, > and that's not the only such pair. BTW, after some grepping: you seem to leave ecryptfs_getattr() as-is, which means that flags it had been passed by caller in *stat are not seen by underlying fs. Looks fishy...
[toc] | [prev] | [next] | [standalone]
| From | David Howells <dhowells@redhat.com> |
|---|---|
| Date | 2016-12-05 16:40 +0100 |
| Subject | Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available [ver #3] |
| Message-ID | <sL3wZ-6hT-3@gated-at.bofh.it> |
| In reply to | #1535696 |
Al Viro <viro@ZenIV.linux.org.uk> wrote: > > I understand wanting to avoid extra arguments, but you are asking for > > trouble with that sort of calling conventions. Verifying that all call > > chains have these fields initialized is bloody unpleasant and it *is* > > going to break, especially since the rules are "you need to initialize it > > for vfs_xgetattr(), but not for vfs_getattr()" - the names are similar > > enough for confusion, and that's not the only such pair. > > BTW, after some grepping: you seem to leave ecryptfs_getattr() as-is, which > means that flags it had been passed by caller in *stat are not seen by > underlying fs. Looks fishy... I have patches for some other filesystems as well, but I'm trying to nail the core down before whipping those back into shape. David
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-12-04 18:40 +0100 |
| Message-ID | <sKIVA-1GG-3@gated-at.bofh.it> |
| In reply to | #1535583 |
On Sun, Dec 04, 2016 at 04:38:05AM +0000, Al Viro wrote:
> I understand wanting to avoid extra arguments, but you are asking for trouble
> with that sort of calling conventions. Verifying that all call chains have
> these fields initialized is bloody unpleasant and it *is* going to break,
> especially since the rules are "you need to initialize it for vfs_xgetattr(),
> but not for vfs_getattr()" - the names are similar enough for confusion,
> and that's not the only such pair.
FWIW, there's a bit of abuse of struct kstat in overlayfs object
creation paths - for one thing, it ends up with a very small subset
of struct kstat (mode + rdev), for another it also needs link in
case of symlinks and ends up passing it separately.
IMO it would be better to introduce a separate object for that; does anybody
have objections to something like the patch below? In principle, we might
even lift that thing into general API and switch ->mkdir()/->mknod()/->symlink()
to identical calling conventions. Hell knows, perhaps ->create() as well...
Comments?
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 36795ee..17ad2da 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -231,10 +231,15 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
struct inode *udir = upperdir->d_inode;
struct dentry *newdentry = NULL;
struct dentry *upper = NULL;
- umode_t mode = stat->mode;
int err;
const struct cred *old_creds = NULL;
struct cred *new_creds = NULL;
+ struct cattr cattr = {
+ /* Can't properly set mode on creation because of the umask */
+ .mode = stat->mode & S_IFMT,
+ .rdev = stat->rdev,
+ .link = link
+ };
newdentry = ovl_lookup_temp(workdir, dentry);
err = PTR_ERR(newdentry);
@@ -254,10 +259,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
if (new_creds)
old_creds = override_creds(new_creds);
- /* Can't properly set mode on creation because of the umask */
- stat->mode &= S_IFMT;
- err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
- stat->mode = mode;
+ err = ovl_create_real(wdir, newdentry, &cattr, NULL, true);
if (new_creds) {
revert_creds(old_creds);
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 306b6c1..dc60a23 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -75,8 +75,8 @@ static struct dentry *ovl_whiteout(struct dentry *workdir,
}
int ovl_create_real(struct inode *dir, struct dentry *newdentry,
- struct kstat *stat, const char *link,
- struct dentry *hardlink, bool debug)
+ struct cattr *attr, struct dentry *hardlink,
+ bool debug)
{
int err;
@@ -86,13 +86,13 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
if (hardlink) {
err = ovl_do_link(hardlink, dir, newdentry, debug);
} else {
- switch (stat->mode & S_IFMT) {
+ switch (attr->mode & S_IFMT) {
case S_IFREG:
- err = ovl_do_create(dir, newdentry, stat->mode, debug);
+ err = ovl_do_create(dir, newdentry, attr->mode, debug);
break;
case S_IFDIR:
- err = ovl_do_mkdir(dir, newdentry, stat->mode, debug);
+ err = ovl_do_mkdir(dir, newdentry, attr->mode, debug);
break;
case S_IFCHR:
@@ -100,11 +100,11 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
case S_IFIFO:
case S_IFSOCK:
err = ovl_do_mknod(dir, newdentry,
- stat->mode, stat->rdev, debug);
+ attr->mode, attr->rdev, debug);
break;
case S_IFLNK:
- err = ovl_do_symlink(dir, newdentry, link, debug);
+ err = ovl_do_symlink(dir, newdentry, attr->link, debug);
break;
default:
@@ -183,8 +183,7 @@ static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
}
static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
- struct kstat *stat, const char *link,
- struct dentry *hardlink)
+ struct cattr *attr, struct dentry *hardlink)
{
struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
struct inode *udir = upperdir->d_inode;
@@ -192,7 +191,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
int err;
if (!hardlink && !IS_POSIXACL(udir))
- stat->mode &= ~current_umask();
+ attr->mode &= ~current_umask();
inode_lock_nested(udir, I_MUTEX_PARENT);
newdentry = lookup_one_len(dentry->d_name.name, upperdir,
@@ -200,7 +199,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
err = PTR_ERR(newdentry);
if (IS_ERR(newdentry))
goto out_unlock;
- err = ovl_create_real(udir, newdentry, stat, link, hardlink, false);
+ err = ovl_create_real(udir, newdentry, attr, hardlink, false);
if (err)
goto out_dput;
@@ -270,7 +269,8 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
if (IS_ERR(opaquedir))
goto out_unlock;
- err = ovl_create_real(wdir, opaquedir, &stat, NULL, NULL, true);
+ err = ovl_create_real(wdir, opaquedir,
+ &(struct cattr){.mode = stat.mode}, NULL, true);
if (err)
goto out_dput;
@@ -370,8 +370,7 @@ static int ovl_set_upper_acl(struct dentry *upperdentry, const char *name,
}
static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
- struct kstat *stat, const char *link,
- struct dentry *hardlink)
+ struct cattr *cattr, struct dentry *hardlink)
{
struct dentry *workdir = ovl_workdir(dentry);
struct inode *wdir = workdir->d_inode;
@@ -387,7 +386,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
if (!hardlink) {
err = posix_acl_create(dentry->d_parent->d_inode,
- &stat->mode, &default_acl, &acl);
+ &cattr->mode, &default_acl, &acl);
if (err)
return err;
}
@@ -407,7 +406,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
if (IS_ERR(upper))
goto out_dput;
- err = ovl_create_real(wdir, newdentry, stat, link, hardlink, true);
+ err = ovl_create_real(wdir, newdentry, cattr, hardlink, true);
if (err)
goto out_dput2;
@@ -415,10 +414,10 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
* mode could have been mutilated due to umask (e.g. sgid directory)
*/
if (!hardlink &&
- !S_ISLNK(stat->mode) && newdentry->d_inode->i_mode != stat->mode) {
+ !S_ISLNK(cattr->mode) && newdentry->d_inode->i_mode != cattr->mode) {
struct iattr attr = {
.ia_valid = ATTR_MODE,
- .ia_mode = stat->mode,
+ .ia_mode = cattr->mode,
};
inode_lock(newdentry->d_inode);
err = notify_change(newdentry, &attr, NULL);
@@ -438,7 +437,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
goto out_cleanup;
}
- if (!hardlink && S_ISDIR(stat->mode)) {
+ if (!hardlink && S_ISDIR(cattr->mode)) {
err = ovl_set_opaque(newdentry);
if (err)
goto out_cleanup;
@@ -475,8 +474,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
}
static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
- struct kstat *stat, const char *link,
- struct dentry *hardlink)
+ struct cattr *attr, struct dentry *hardlink)
{
int err;
const struct cred *old_cred;
@@ -494,7 +492,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
override_cred->fsgid = inode->i_gid;
if (!hardlink) {
err = security_dentry_create_files_as(dentry,
- stat->mode, &dentry->d_name, old_cred,
+ attr->mode, &dentry->d_name, old_cred,
override_cred);
if (err) {
put_cred(override_cred);
@@ -505,11 +503,11 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
put_cred(override_cred);
if (!ovl_dentry_is_opaque(dentry))
- err = ovl_create_upper(dentry, inode, stat, link,
+ err = ovl_create_upper(dentry, inode, attr,
hardlink);
else
- err = ovl_create_over_whiteout(dentry, inode, stat,
- link, hardlink);
+ err = ovl_create_over_whiteout(dentry, inode, attr,
+ hardlink);
}
out_revert_creds:
revert_creds(old_cred);
@@ -528,8 +526,9 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
{
int err;
struct inode *inode;
- struct kstat stat = {
+ struct cattr attr = {
.rdev = rdev,
+ .link = link,
};
err = ovl_want_write(dentry);
@@ -542,9 +541,9 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
goto out_drop_write;
inode_init_owner(inode, dentry->d_parent->d_inode, mode);
- stat.mode = inode->i_mode;
+ attr.mode = inode->i_mode;
- err = ovl_create_or_link(dentry, inode, &stat, link, NULL);
+ err = ovl_create_or_link(dentry, inode, &attr, NULL);
if (err)
iput(inode);
@@ -598,7 +597,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
inode = d_inode(old);
ihold(inode);
- err = ovl_create_or_link(new, inode, NULL, NULL, ovl_dentry_upper(old));
+ err = ovl_create_or_link(new, inode, NULL, ovl_dentry_upper(old));
if (err)
iput(inode);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index e218e74..346b38b 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -210,8 +210,13 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
/* dir.c */
extern const struct inode_operations ovl_dir_inode_operations;
struct dentry *ovl_lookup_temp(struct dentry *workdir, struct dentry *dentry);
+struct cattr {
+ dev_t rdev;
+ umode_t mode;
+ const char *link;
+};
int ovl_create_real(struct inode *dir, struct dentry *newdentry,
- struct kstat *stat, const char *link,
+ struct cattr *attr,
struct dentry *hardlink, bool debug);
void ovl_cleanup(struct inode *dir, struct dentry *dentry);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index edd46a0..c95e447 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -809,12 +809,9 @@ static struct dentry *ovl_workdir_create(struct vfsmount *mnt,
strlen(OVL_WORKDIR_NAME));
if (!IS_ERR(work)) {
- struct kstat stat = {
- .mode = S_IFDIR | 0,
- };
struct iattr attr = {
.ia_valid = ATTR_MODE,
- .ia_mode = stat.mode,
+ .ia_mode = S_IFDIR | 0,
};
if (work->d_inode) {
@@ -828,7 +825,9 @@ static struct dentry *ovl_workdir_create(struct vfsmount *mnt,
goto retry;
}
- err = ovl_create_real(dir, work, &stat, NULL, NULL, true);
+ err = ovl_create_real(dir, work,
+ &(struct cattr){.mode = S_IFDIR | 0},
+ NULL, true);
if (err)
goto out_dput;
[toc] | [prev] | [next] | [standalone]
| From | Miklos Szeredi <miklos@szeredi.hu> |
|---|---|
| Date | 2016-12-05 15:50 +0100 |
| Message-ID | <sL2KC-5N1-23@gated-at.bofh.it> |
| In reply to | #1535700 |
On Sun, Dec 4, 2016 at 6:33 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Sun, Dec 04, 2016 at 04:38:05AM +0000, Al Viro wrote: > >> I understand wanting to avoid extra arguments, but you are asking for trouble >> with that sort of calling conventions. Verifying that all call chains have >> these fields initialized is bloody unpleasant and it *is* going to break, >> especially since the rules are "you need to initialize it for vfs_xgetattr(), >> but not for vfs_getattr()" - the names are similar enough for confusion, >> and that's not the only such pair. > > FWIW, there's a bit of abuse of struct kstat in overlayfs object > creation paths - for one thing, it ends up with a very small subset > of struct kstat (mode + rdev), for another it also needs link in > case of symlinks and ends up passing it separately. > > IMO it would be better to introduce a separate object for that; does anybody > have objections to something like the patch below? In principle, we might > even lift that thing into general API and switch ->mkdir()/->mknod()/->symlink() > to identical calling conventions. Hell knows, perhaps ->create() as well... > Comments? Good cleanup. Applied, thanks. Miklos
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web