| Message ID | 1508421517-22678-5-git-send-email-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 19, 2017 at 4:58 PM, Miklos Szeredi <mszeredi@redhat.com> wrote: > Use helpers to get first and next marks from connector. > > Also get rid of inode_node/vfsmount_node local variables, which just refers > to the same objects as iter_info. There was an srcu_dereference() for > foo_node, but that's completely superfluous since we've already done it > when obtaining foo_node. > > Also get rid of inode_group/vfsmount_group local variables; checking > against non-NULL for these is the same as checking against non-NULL > inode_mark/vfsmount_mark. > Nice Cleanup! > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- > fs/notify/fsnotify.c | 107 +++++++++++++++++++++++---------------------------- > 1 file changed, 48 insertions(+), 59 deletions(-) > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index 0ab6a7179e4d..7490edd74883 100644 > --- a/fs/notify/fsnotify.c > +++ b/fs/notify/fsnotify.c > @@ -243,6 +243,28 @@ static int send_to_group(struct inode *to_tell, > file_name, cookie, iter_info); > } > > +static struct fsnotify_mark *fsnotify_first_mark(struct fsnotify_mark_connector **connp) > +{ > + struct fsnotify_mark_connector *conn; > + struct hlist_node *node = NULL; > + > + conn = srcu_dereference(*connp, &fsnotify_mark_srcu); > + if (conn) > + node = srcu_dereference(conn->list.first, &fsnotify_mark_srcu); > + > + return hlist_entry_safe(node, struct fsnotify_mark, obj_list); > +} > + > +static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark) > +{ > + struct hlist_node *node = NULL; > + > + if (mark) > + node = srcu_dereference(mark->obj_list.next, &fsnotify_mark_srcu); > + > + return hlist_entry_safe(node, struct fsnotify_mark, obj_list); > +} > + > /* > * This is the main call to fsnotify. The VFS calls into hook specific functions > * in linux/fsnotify.h. Those functions then in turn call here. Here will call > @@ -252,11 +274,7 @@ static int send_to_group(struct inode *to_tell, > int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, > const unsigned char *file_name, u32 cookie) > { > - struct hlist_node *inode_node = NULL, *vfsmount_node = NULL; > - struct fsnotify_mark *inode_mark = NULL, *vfsmount_mark = NULL; > - struct fsnotify_group *inode_group, *vfsmount_group; > - struct fsnotify_mark_connector *inode_conn, *vfsmount_conn; > - struct fsnotify_iter_info iter_info; > + struct fsnotify_iter_info iter_info = { NULL, NULL }; There was a series of security patches converting those initializers to { } don't ask me why. > struct mount *mnt; > int ret = 0; > /* global tests shouldn't care about events on child only the specific event */ > @@ -291,26 +309,13 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, > > if ((mask & FS_MODIFY) || > (test_mask & to_tell->i_fsnotify_mask)) { > - inode_conn = srcu_dereference(to_tell->i_fsnotify_marks, > - &fsnotify_mark_srcu); > - if (inode_conn) > - inode_node = srcu_dereference(inode_conn->list.first, > - &fsnotify_mark_srcu); > + iter_info.inode_mark = fsnotify_first_mark(&to_tell->i_fsnotify_marks); > } > > if (mnt && ((mask & FS_MODIFY) || > (test_mask & mnt->mnt_fsnotify_mask))) { > - inode_conn = srcu_dereference(to_tell->i_fsnotify_marks, > - &fsnotify_mark_srcu); > - if (inode_conn) > - inode_node = srcu_dereference(inode_conn->list.first, > - &fsnotify_mark_srcu); > - vfsmount_conn = srcu_dereference(mnt->mnt_fsnotify_marks, > - &fsnotify_mark_srcu); > - if (vfsmount_conn) > - vfsmount_node = srcu_dereference( > - vfsmount_conn->list.first, > - &fsnotify_mark_srcu); > + iter_info.inode_mark = fsnotify_first_mark(&to_tell->i_fsnotify_marks); > + iter_info.vfsmount_mark = fsnotify_first_mark(&mnt->mnt_fsnotify_marks); > } > > /* > @@ -318,41 +323,28 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, > * ignore masks are properly reflected for mount mark notifications. > * That's why this traversal is so complicated... > */ > - while (inode_node || vfsmount_node) { > - inode_group = NULL; > - inode_mark = NULL; > - vfsmount_group = NULL; > - vfsmount_mark = NULL; > - > - if (inode_node) { > - inode_mark = hlist_entry(srcu_dereference(inode_node, &fsnotify_mark_srcu), > - struct fsnotify_mark, obj_list); > - inode_group = inode_mark->group; > - if (!(inode_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) > - goto skip_inode; > - } > + while (iter_info.inode_mark || iter_info.vfsmount_mark) { > + struct fsnotify_mark *inode_mark = iter_info.inode_mark; > + struct fsnotify_mark *vfsmount_mark = iter_info.vfsmount_mark; > > - if (vfsmount_node) { > - vfsmount_mark = hlist_entry(srcu_dereference(vfsmount_node, &fsnotify_mark_srcu), > - struct fsnotify_mark, obj_list); > - vfsmount_group = vfsmount_mark->group; > - if (!(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) > - goto skip_vfsmount; > + if (inode_mark && > + !(inode_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { > + vfsmount_mark = NULL; > + goto skip; > + } > + if (vfsmount_mark && > + !(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { > + inode_mark = NULL; > + goto skip; > } > > - iter_info.inode_mark = inode_mark; > - iter_info.vfsmount_mark = vfsmount_mark; > - > - if (inode_group && vfsmount_group) { > - int cmp = fsnotify_compare_groups(inode_group, > - vfsmount_group); > - if (cmp > 0) { > - inode_group = NULL; > + if (inode_mark && vfsmount_mark) { > + int cmp = fsnotify_compare_groups(inode_mark->group, > + vfsmount_mark->group); > + if (cmp > 0) > inode_mark = NULL; > - } else if (cmp < 0) { > - vfsmount_group = NULL; > + else if (cmp < 0) > vfsmount_mark = NULL; > - } > } > > ret = send_to_group(to_tell, inode_mark, vfsmount_mark, mask, > @@ -361,14 +353,11 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, > > if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS)) > goto out; > -skip_inode: > - if (inode_group) > - inode_node = srcu_dereference(inode_node->next, > - &fsnotify_mark_srcu); > -skip_vfsmount: > - if (vfsmount_group) > - vfsmount_node = srcu_dereference(vfsmount_node->next, > - &fsnotify_mark_srcu); > +skip: > + if (inode_mark) > + iter_info.inode_mark = fsnotify_next_mark(iter_info.inode_mark); else if? > + if (vfsmount_mark) > + iter_info.vfsmount_mark = fsnotify_next_mark(iter_info.vfsmount_mark); > } > ret = 0; > out: > -- > 2.5.5 >
On Fri, Oct 20, 2017 at 2:48 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Thu, Oct 19, 2017 at 4:58 PM, Miklos Szeredi <mszeredi@redhat.com> wrote: >> Use helpers to get first and next marks from connector. >> >> Also get rid of inode_node/vfsmount_node local variables, which just refers >> to the same objects as iter_info. There was an srcu_dereference() for >> foo_node, but that's completely superfluous since we've already done it >> when obtaining foo_node. >> >> Also get rid of inode_group/vfsmount_group local variables; checking >> against non-NULL for these is the same as checking against non-NULL >> inode_mark/vfsmount_mark. >> > > Nice Cleanup! > >> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> >> --- >> fs/notify/fsnotify.c | 107 +++++++++++++++++++++++---------------------------- >> 1 file changed, 48 insertions(+), 59 deletions(-) >> >> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c >> index 0ab6a7179e4d..7490edd74883 100644 >> --- a/fs/notify/fsnotify.c >> +++ b/fs/notify/fsnotify.c >> @@ -243,6 +243,28 @@ static int send_to_group(struct inode *to_tell, >> file_name, cookie, iter_info); >> } >> >> +static struct fsnotify_mark *fsnotify_first_mark(struct fsnotify_mark_connector **connp) >> +{ >> + struct fsnotify_mark_connector *conn; >> + struct hlist_node *node = NULL; >> + >> + conn = srcu_dereference(*connp, &fsnotify_mark_srcu); >> + if (conn) >> + node = srcu_dereference(conn->list.first, &fsnotify_mark_srcu); >> + >> + return hlist_entry_safe(node, struct fsnotify_mark, obj_list); >> +} >> + >> +static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark) >> +{ >> + struct hlist_node *node = NULL; >> + >> + if (mark) >> + node = srcu_dereference(mark->obj_list.next, &fsnotify_mark_srcu); >> + >> + return hlist_entry_safe(node, struct fsnotify_mark, obj_list); >> +} >> + >> /* >> * This is the main call to fsnotify. The VFS calls into hook specific functions >> * in linux/fsnotify.h. Those functions then in turn call here. Here will call >> @@ -252,11 +274,7 @@ static int send_to_group(struct inode *to_tell, >> int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, >> const unsigned char *file_name, u32 cookie) >> { >> - struct hlist_node *inode_node = NULL, *vfsmount_node = NULL; >> - struct fsnotify_mark *inode_mark = NULL, *vfsmount_mark = NULL; >> - struct fsnotify_group *inode_group, *vfsmount_group; >> - struct fsnotify_mark_connector *inode_conn, *vfsmount_conn; >> - struct fsnotify_iter_info iter_info; >> + struct fsnotify_iter_info iter_info = { NULL, NULL }; > > There was a series of security patches converting those initializers to { } > don't ask me why. Okay. > >> struct mount *mnt; >> int ret = 0; >> /* global tests shouldn't care about events on child only the specific event */ >> @@ -291,26 +309,13 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, >> >> if ((mask & FS_MODIFY) || >> (test_mask & to_tell->i_fsnotify_mask)) { >> - inode_conn = srcu_dereference(to_tell->i_fsnotify_marks, >> - &fsnotify_mark_srcu); >> - if (inode_conn) >> - inode_node = srcu_dereference(inode_conn->list.first, >> - &fsnotify_mark_srcu); >> + iter_info.inode_mark = fsnotify_first_mark(&to_tell->i_fsnotify_marks); >> } >> >> if (mnt && ((mask & FS_MODIFY) || >> (test_mask & mnt->mnt_fsnotify_mask))) { >> - inode_conn = srcu_dereference(to_tell->i_fsnotify_marks, >> - &fsnotify_mark_srcu); >> - if (inode_conn) >> - inode_node = srcu_dereference(inode_conn->list.first, >> - &fsnotify_mark_srcu); >> - vfsmount_conn = srcu_dereference(mnt->mnt_fsnotify_marks, >> - &fsnotify_mark_srcu); >> - if (vfsmount_conn) >> - vfsmount_node = srcu_dereference( >> - vfsmount_conn->list.first, >> - &fsnotify_mark_srcu); >> + iter_info.inode_mark = fsnotify_first_mark(&to_tell->i_fsnotify_marks); >> + iter_info.vfsmount_mark = fsnotify_first_mark(&mnt->mnt_fsnotify_marks); >> } >> >> /* >> @@ -318,41 +323,28 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, >> * ignore masks are properly reflected for mount mark notifications. >> * That's why this traversal is so complicated... >> */ >> - while (inode_node || vfsmount_node) { >> - inode_group = NULL; >> - inode_mark = NULL; >> - vfsmount_group = NULL; >> - vfsmount_mark = NULL; >> - >> - if (inode_node) { >> - inode_mark = hlist_entry(srcu_dereference(inode_node, &fsnotify_mark_srcu), >> - struct fsnotify_mark, obj_list); >> - inode_group = inode_mark->group; >> - if (!(inode_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) >> - goto skip_inode; >> - } >> + while (iter_info.inode_mark || iter_info.vfsmount_mark) { >> + struct fsnotify_mark *inode_mark = iter_info.inode_mark; >> + struct fsnotify_mark *vfsmount_mark = iter_info.vfsmount_mark; >> >> - if (vfsmount_node) { >> - vfsmount_mark = hlist_entry(srcu_dereference(vfsmount_node, &fsnotify_mark_srcu), >> - struct fsnotify_mark, obj_list); >> - vfsmount_group = vfsmount_mark->group; >> - if (!(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) >> - goto skip_vfsmount; >> + if (inode_mark && >> + !(inode_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { >> + vfsmount_mark = NULL; >> + goto skip; >> + } >> + if (vfsmount_mark && >> + !(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { >> + inode_mark = NULL; >> + goto skip; >> } >> >> - iter_info.inode_mark = inode_mark; >> - iter_info.vfsmount_mark = vfsmount_mark; >> - >> - if (inode_group && vfsmount_group) { >> - int cmp = fsnotify_compare_groups(inode_group, >> - vfsmount_group); >> - if (cmp > 0) { >> - inode_group = NULL; >> + if (inode_mark && vfsmount_mark) { >> + int cmp = fsnotify_compare_groups(inode_mark->group, >> + vfsmount_mark->group); >> + if (cmp > 0) >> inode_mark = NULL; >> - } else if (cmp < 0) { >> - vfsmount_group = NULL; >> + else if (cmp < 0) >> vfsmount_mark = NULL; >> - } >> } >> >> ret = send_to_group(to_tell, inode_mark, vfsmount_mark, mask, >> @@ -361,14 +353,11 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, >> >> if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS)) >> goto out; >> -skip_inode: >> - if (inode_group) >> - inode_node = srcu_dereference(inode_node->next, >> - &fsnotify_mark_srcu); >> -skip_vfsmount: >> - if (vfsmount_group) >> - vfsmount_node = srcu_dereference(vfsmount_node->next, >> - &fsnotify_mark_srcu); >> +skip: >> + if (inode_mark) >> + iter_info.inode_mark = fsnotify_next_mark(iter_info.inode_mark); > else if? We can have both (see fsnotify_compare_groups()). >> + if (vfsmount_mark) >> + iter_info.vfsmount_mark = fsnotify_next_mark(iter_info.vfsmount_mark); >> } >> ret = 0; >> out: >> -- >> 2.5.5 >>
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index 0ab6a7179e4d..7490edd74883 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -243,6 +243,28 @@ static int send_to_group(struct inode *to_tell, file_name, cookie, iter_info); } +static struct fsnotify_mark *fsnotify_first_mark(struct fsnotify_mark_connector **connp) +{ + struct fsnotify_mark_connector *conn; + struct hlist_node *node = NULL; + + conn = srcu_dereference(*connp, &fsnotify_mark_srcu); + if (conn) + node = srcu_dereference(conn->list.first, &fsnotify_mark_srcu); + + return hlist_entry_safe(node, struct fsnotify_mark, obj_list); +} + +static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark) +{ + struct hlist_node *node = NULL; + + if (mark) + node = srcu_dereference(mark->obj_list.next, &fsnotify_mark_srcu); + + return hlist_entry_safe(node, struct fsnotify_mark, obj_list); +} + /* * This is the main call to fsnotify. The VFS calls into hook specific functions * in linux/fsnotify.h. Those functions then in turn call here. Here will call @@ -252,11 +274,7 @@ static int send_to_group(struct inode *to_tell, int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, const unsigned char *file_name, u32 cookie) { - struct hlist_node *inode_node = NULL, *vfsmount_node = NULL; - struct fsnotify_mark *inode_mark = NULL, *vfsmount_mark = NULL; - struct fsnotify_group *inode_group, *vfsmount_group; - struct fsnotify_mark_connector *inode_conn, *vfsmount_conn; - struct fsnotify_iter_info iter_info; + struct fsnotify_iter_info iter_info = { NULL, NULL }; struct mount *mnt; int ret = 0; /* global tests shouldn't care about events on child only the specific event */ @@ -291,26 +309,13 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, if ((mask & FS_MODIFY) || (test_mask & to_tell->i_fsnotify_mask)) { - inode_conn = srcu_dereference(to_tell->i_fsnotify_marks, - &fsnotify_mark_srcu); - if (inode_conn) - inode_node = srcu_dereference(inode_conn->list.first, - &fsnotify_mark_srcu); + iter_info.inode_mark = fsnotify_first_mark(&to_tell->i_fsnotify_marks); } if (mnt && ((mask & FS_MODIFY) || (test_mask & mnt->mnt_fsnotify_mask))) { - inode_conn = srcu_dereference(to_tell->i_fsnotify_marks, - &fsnotify_mark_srcu); - if (inode_conn) - inode_node = srcu_dereference(inode_conn->list.first, - &fsnotify_mark_srcu); - vfsmount_conn = srcu_dereference(mnt->mnt_fsnotify_marks, - &fsnotify_mark_srcu); - if (vfsmount_conn) - vfsmount_node = srcu_dereference( - vfsmount_conn->list.first, - &fsnotify_mark_srcu); + iter_info.inode_mark = fsnotify_first_mark(&to_tell->i_fsnotify_marks); + iter_info.vfsmount_mark = fsnotify_first_mark(&mnt->mnt_fsnotify_marks); } /* @@ -318,41 +323,28 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, * ignore masks are properly reflected for mount mark notifications. * That's why this traversal is so complicated... */ - while (inode_node || vfsmount_node) { - inode_group = NULL; - inode_mark = NULL; - vfsmount_group = NULL; - vfsmount_mark = NULL; - - if (inode_node) { - inode_mark = hlist_entry(srcu_dereference(inode_node, &fsnotify_mark_srcu), - struct fsnotify_mark, obj_list); - inode_group = inode_mark->group; - if (!(inode_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) - goto skip_inode; - } + while (iter_info.inode_mark || iter_info.vfsmount_mark) { + struct fsnotify_mark *inode_mark = iter_info.inode_mark; + struct fsnotify_mark *vfsmount_mark = iter_info.vfsmount_mark; - if (vfsmount_node) { - vfsmount_mark = hlist_entry(srcu_dereference(vfsmount_node, &fsnotify_mark_srcu), - struct fsnotify_mark, obj_list); - vfsmount_group = vfsmount_mark->group; - if (!(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) - goto skip_vfsmount; + if (inode_mark && + !(inode_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { + vfsmount_mark = NULL; + goto skip; + } + if (vfsmount_mark && + !(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { + inode_mark = NULL; + goto skip; } - iter_info.inode_mark = inode_mark; - iter_info.vfsmount_mark = vfsmount_mark; - - if (inode_group && vfsmount_group) { - int cmp = fsnotify_compare_groups(inode_group, - vfsmount_group); - if (cmp > 0) { - inode_group = NULL; + if (inode_mark && vfsmount_mark) { + int cmp = fsnotify_compare_groups(inode_mark->group, + vfsmount_mark->group); + if (cmp > 0) inode_mark = NULL; - } else if (cmp < 0) { - vfsmount_group = NULL; + else if (cmp < 0) vfsmount_mark = NULL; - } } ret = send_to_group(to_tell, inode_mark, vfsmount_mark, mask, @@ -361,14 +353,11 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS)) goto out; -skip_inode: - if (inode_group) - inode_node = srcu_dereference(inode_node->next, - &fsnotify_mark_srcu); -skip_vfsmount: - if (vfsmount_group) - vfsmount_node = srcu_dereference(vfsmount_node->next, - &fsnotify_mark_srcu); +skip: + if (inode_mark) + iter_info.inode_mark = fsnotify_next_mark(iter_info.inode_mark); + if (vfsmount_mark) + iter_info.vfsmount_mark = fsnotify_next_mark(iter_info.vfsmount_mark); } ret = 0; out:
Use helpers to get first and next marks from connector. Also get rid of inode_node/vfsmount_node local variables, which just refers to the same objects as iter_info. There was an srcu_dereference() for foo_node, but that's completely superfluous since we've already done it when obtaining foo_node. Also get rid of inode_group/vfsmount_group local variables; checking against non-NULL for these is the same as checking against non-NULL inode_mark/vfsmount_mark. Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- fs/notify/fsnotify.c | 107 +++++++++++++++++++++++---------------------------- 1 file changed, 48 insertions(+), 59 deletions(-)