On Fri, Oct 20, 2017 at 2:48 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> On Thu, Oct 19, 2017 at 4:58 PM, Miklos Szeredi <mszeredi@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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
>>
