| Message ID | 20161222091538.28702-7-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 22, 2016 at 4:15 AM, Jan Kara <jack@suse.cz> wrote: > Audit tree currently uses inode pointer as a key into the hash table. > Getting that from notification mark will be somewhat more difficult with > coming fsnotify changes and there's no reason we really have to use the > inode pointer. So abstract getting of hash key from the audit chunk and > inode so that we can switch to a different key easily later. > > CC: Paul Moore <paul@paul-moore.com> > Signed-off-by: Jan Kara <jack@suse.cz> > --- > kernel/audit_tree.c | 39 ++++++++++++++++++++++++++++----------- > 1 file changed, 28 insertions(+), 11 deletions(-) I have no objections with this patch in particular, but in patch 8, are you certain that inode_to_key() and chunk_to_key() will continue to return the same key value? > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > index 156b6a93f4fc..f0859828de09 100644 > --- a/kernel/audit_tree.c > +++ b/kernel/audit_tree.c > @@ -163,33 +163,48 @@ enum {HASH_SIZE = 128}; > static struct list_head chunk_hash_heads[HASH_SIZE]; > static __cacheline_aligned_in_smp DEFINE_SPINLOCK(hash_lock); > > -static inline struct list_head *chunk_hash(const struct inode *inode) > +/* Function to return search key in our hash from inode. */ > +static unsigned long inode_to_key(const struct inode *inode) > { > - unsigned long n = (unsigned long)inode / L1_CACHE_BYTES; > + return (unsigned long)inode; > +} > + > +/* > + * Function to return search key in our hash from chunk. Key 0 is special and > + * should never be present in the hash. > + */ > +static unsigned long chunk_to_key(struct audit_chunk *chunk) > +{ > + return (unsigned long)chunk->mark.inode; > +} > + > +static inline struct list_head *chunk_hash(unsigned long key) > +{ > + unsigned long n = key / L1_CACHE_BYTES; > return chunk_hash_heads + n % HASH_SIZE; > } > > /* hash_lock & entry->lock is held by caller */ > static void insert_hash(struct audit_chunk *chunk) > { > - struct fsnotify_mark *entry = &chunk->mark; > + unsigned long key = chunk_to_key(chunk); > struct list_head *list; > > - if (!entry->inode) > + if (!key) > return; > - list = chunk_hash(entry->inode); > + list = chunk_hash(key); > list_add_rcu(&chunk->hash, list); > } > > /* called under rcu_read_lock */ > struct audit_chunk *audit_tree_lookup(const struct inode *inode) > { > - struct list_head *list = chunk_hash(inode); > + unsigned long key = inode_to_key(inode); > + struct list_head *list = chunk_hash(key); > struct audit_chunk *p; > > list_for_each_entry_rcu(p, list, hash) { > - /* mark.inode may have gone NULL, but who cares? */ > - if (p->mark.inode == inode) { > + if (chunk_to_key(p) == key) { > atomic_long_inc(&p->refs); > return p; > } > @@ -585,7 +600,8 @@ int audit_remove_tree_rule(struct audit_krule *rule) > > static int compare_root(struct vfsmount *mnt, void *arg) > { > - return d_backing_inode(mnt->mnt_root) == arg; > + return inode_to_key(d_backing_inode(mnt->mnt_root)) == > + (unsigned long)arg; > } > > void audit_trim_trees(void) > @@ -620,9 +636,10 @@ void audit_trim_trees(void) > list_for_each_entry(node, &tree->chunks, list) { > struct audit_chunk *chunk = find_chunk(node); > /* this could be NULL if the watch is dying else where... */ > - struct inode *inode = chunk->mark.inode; > node->index |= 1U<<31; > - if (iterate_mounts(compare_root, inode, root_mnt)) > + if (iterate_mounts(compare_root, > + (void *)chunk_to_key(chunk), > + root_mnt)) > node->index &= ~(1U<<31); > } > spin_unlock(&hash_lock); > -- > 2.10.2 >
On Thu 22-12-16 18:27:40, Paul Moore wrote: > On Thu, Dec 22, 2016 at 4:15 AM, Jan Kara <jack@suse.cz> wrote: > > Audit tree currently uses inode pointer as a key into the hash table. > > Getting that from notification mark will be somewhat more difficult with > > coming fsnotify changes and there's no reason we really have to use the > > inode pointer. So abstract getting of hash key from the audit chunk and > > inode so that we can switch to a different key easily later. > > > > CC: Paul Moore <paul@paul-moore.com> > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > kernel/audit_tree.c | 39 ++++++++++++++++++++++++++++----------- > > 1 file changed, 28 insertions(+), 11 deletions(-) > > I have no objections with this patch in particular, but in patch 8, > are you certain that inode_to_key() and chunk_to_key() will continue > to return the same key value? Yes, that's the intention. Or better in that patch the key will no longer be inode pointer but instead the fsnotify_list pointer. But still it would match for chunks attached to an inode and inode itself so comparison results should stay the same. Honza > > > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > > index 156b6a93f4fc..f0859828de09 100644 > > --- a/kernel/audit_tree.c > > +++ b/kernel/audit_tree.c > > @@ -163,33 +163,48 @@ enum {HASH_SIZE = 128}; > > static struct list_head chunk_hash_heads[HASH_SIZE]; > > static __cacheline_aligned_in_smp DEFINE_SPINLOCK(hash_lock); > > > > -static inline struct list_head *chunk_hash(const struct inode *inode) > > +/* Function to return search key in our hash from inode. */ > > +static unsigned long inode_to_key(const struct inode *inode) > > { > > - unsigned long n = (unsigned long)inode / L1_CACHE_BYTES; > > + return (unsigned long)inode; > > +} > > + > > +/* > > + * Function to return search key in our hash from chunk. Key 0 is special and > > + * should never be present in the hash. > > + */ > > +static unsigned long chunk_to_key(struct audit_chunk *chunk) > > +{ > > + return (unsigned long)chunk->mark.inode; > > +} > > + > > +static inline struct list_head *chunk_hash(unsigned long key) > > +{ > > + unsigned long n = key / L1_CACHE_BYTES; > > return chunk_hash_heads + n % HASH_SIZE; > > } > > > > /* hash_lock & entry->lock is held by caller */ > > static void insert_hash(struct audit_chunk *chunk) > > { > > - struct fsnotify_mark *entry = &chunk->mark; > > + unsigned long key = chunk_to_key(chunk); > > struct list_head *list; > > > > - if (!entry->inode) > > + if (!key) > > return; > > - list = chunk_hash(entry->inode); > > + list = chunk_hash(key); > > list_add_rcu(&chunk->hash, list); > > } > > > > /* called under rcu_read_lock */ > > struct audit_chunk *audit_tree_lookup(const struct inode *inode) > > { > > - struct list_head *list = chunk_hash(inode); > > + unsigned long key = inode_to_key(inode); > > + struct list_head *list = chunk_hash(key); > > struct audit_chunk *p; > > > > list_for_each_entry_rcu(p, list, hash) { > > - /* mark.inode may have gone NULL, but who cares? */ > > - if (p->mark.inode == inode) { > > + if (chunk_to_key(p) == key) { > > atomic_long_inc(&p->refs); > > return p; > > } > > @@ -585,7 +600,8 @@ int audit_remove_tree_rule(struct audit_krule *rule) > > > > static int compare_root(struct vfsmount *mnt, void *arg) > > { > > - return d_backing_inode(mnt->mnt_root) == arg; > > + return inode_to_key(d_backing_inode(mnt->mnt_root)) == > > + (unsigned long)arg; > > } > > > > void audit_trim_trees(void) > > @@ -620,9 +636,10 @@ void audit_trim_trees(void) > > list_for_each_entry(node, &tree->chunks, list) { > > struct audit_chunk *chunk = find_chunk(node); > > /* this could be NULL if the watch is dying else where... */ > > - struct inode *inode = chunk->mark.inode; > > node->index |= 1U<<31; > > - if (iterate_mounts(compare_root, inode, root_mnt)) > > + if (iterate_mounts(compare_root, > > + (void *)chunk_to_key(chunk), > > + root_mnt)) > > node->index &= ~(1U<<31); > > } > > spin_unlock(&hash_lock); > > -- > > 2.10.2 > > > > > > -- > paul moore > www.paul-moore.com
On Fri, Dec 23, 2016 at 8:27 AM, Jan Kara <jack@suse.cz> wrote: > On Thu 22-12-16 18:27:40, Paul Moore wrote: >> On Thu, Dec 22, 2016 at 4:15 AM, Jan Kara <jack@suse.cz> wrote: >> > Audit tree currently uses inode pointer as a key into the hash table. >> > Getting that from notification mark will be somewhat more difficult with >> > coming fsnotify changes and there's no reason we really have to use the >> > inode pointer. So abstract getting of hash key from the audit chunk and >> > inode so that we can switch to a different key easily later. >> > >> > CC: Paul Moore <paul@paul-moore.com> >> > Signed-off-by: Jan Kara <jack@suse.cz> >> > --- >> > kernel/audit_tree.c | 39 ++++++++++++++++++++++++++++----------- >> > 1 file changed, 28 insertions(+), 11 deletions(-) >> >> I have no objections with this patch in particular, but in patch 8, >> are you certain that inode_to_key() and chunk_to_key() will continue >> to return the same key value? > > Yes, that's the intention. Or better in that patch the key will no longer > be inode pointer but instead the fsnotify_list pointer. But still it would > match for chunks attached to an inode and inode itself so comparison > results should stay the same. My apologies, I probably should have been more clear. Yes, I think we are all in agreement that the *_to_key() functions need to return a consistent value for the same object. My concern is that in patch 8 these functions are using different variables (granted they may contain the same value, and therefore evaluate to the same key) and I worry that there is a possibility of the two variables taking on different values and breaking the hash. What guarantees exist that these values will be the same? Are there any safeguards to prevent future patches from accidentally sidestepping these guarantees?
On Fri 23-12-16 09:13:55, Paul Moore wrote: > On Fri, Dec 23, 2016 at 8:27 AM, Jan Kara <jack@suse.cz> wrote: > > On Thu 22-12-16 18:27:40, Paul Moore wrote: > >> On Thu, Dec 22, 2016 at 4:15 AM, Jan Kara <jack@suse.cz> wrote: > >> > Audit tree currently uses inode pointer as a key into the hash table. > >> > Getting that from notification mark will be somewhat more difficult with > >> > coming fsnotify changes and there's no reason we really have to use the > >> > inode pointer. So abstract getting of hash key from the audit chunk and > >> > inode so that we can switch to a different key easily later. > >> > > >> > CC: Paul Moore <paul@paul-moore.com> > >> > Signed-off-by: Jan Kara <jack@suse.cz> > >> > --- > >> > kernel/audit_tree.c | 39 ++++++++++++++++++++++++++++----------- > >> > 1 file changed, 28 insertions(+), 11 deletions(-) > >> > >> I have no objections with this patch in particular, but in patch 8, > >> are you certain that inode_to_key() and chunk_to_key() will continue > >> to return the same key value? > > > > Yes, that's the intention. Or better in that patch the key will no longer > > be inode pointer but instead the fsnotify_list pointer. But still it would > > match for chunks attached to an inode and inode itself so comparison > > results should stay the same. > > My apologies, I probably should have been more clear. > > Yes, I think we are all in agreement that the *_to_key() functions > need to return a consistent value for the same object. My concern is > that in patch 8 these functions are using different variables (granted > they may contain the same value, and therefore evaluate to the same > key) and I worry that there is a possibility of the two variables > taking on different values and breaking the hash. What guarantees > exist that these values will be the same? Are there any safeguards to > prevent future patches from accidentally sidestepping these > guarantees? Ah, OK, so this is more about patch 8 than patch 6. So far audit uses inode pointer as a key - now with patch 8, there is a fsnotify_mark_list attached to an inode if and only if there is any fsnotify_mark for that inode and both inode->i_fsnotify_marks (used as a key in inode_to_key()) and mark->obj_list_head (used as a key in chunk_to_key()) point to it. So keys for an inode and chunk match if and only if the fsnotify mark in the chunk is attached to the inode - the same as before patch 8. The only reason why I changed audit to use a different pointer for the key is that you need some lock protection to do mark->obj_list_head->inode dereference and this seemed the easiest. Actually now that all the lifetime rules have worked out, I can see we can actually use inode pointer as a key relatively easily since mark->obj_list_head is stable once you hold a mark reference so locking would be only intermediate step until this gets established in the series. Would you prefer me to do that? Honza
On Tue, Jan 3, 2017 at 12:34 PM, Jan Kara <jack@suse.cz> wrote: > On Fri 23-12-16 09:13:55, Paul Moore wrote: >> On Fri, Dec 23, 2016 at 8:27 AM, Jan Kara <jack@suse.cz> wrote: >> > On Thu 22-12-16 18:27:40, Paul Moore wrote: >> >> On Thu, Dec 22, 2016 at 4:15 AM, Jan Kara <jack@suse.cz> wrote: >> >> > Audit tree currently uses inode pointer as a key into the hash table. >> >> > Getting that from notification mark will be somewhat more difficult with >> >> > coming fsnotify changes and there's no reason we really have to use the >> >> > inode pointer. So abstract getting of hash key from the audit chunk and >> >> > inode so that we can switch to a different key easily later. >> >> > >> >> > CC: Paul Moore <paul@paul-moore.com> >> >> > Signed-off-by: Jan Kara <jack@suse.cz> >> >> > --- >> >> > kernel/audit_tree.c | 39 ++++++++++++++++++++++++++++----------- >> >> > 1 file changed, 28 insertions(+), 11 deletions(-) >> >> >> >> I have no objections with this patch in particular, but in patch 8, >> >> are you certain that inode_to_key() and chunk_to_key() will continue >> >> to return the same key value? >> > >> > Yes, that's the intention. Or better in that patch the key will no longer >> > be inode pointer but instead the fsnotify_list pointer. But still it would >> > match for chunks attached to an inode and inode itself so comparison >> > results should stay the same. >> >> My apologies, I probably should have been more clear. >> >> Yes, I think we are all in agreement that the *_to_key() functions >> need to return a consistent value for the same object. My concern is >> that in patch 8 these functions are using different variables (granted >> they may contain the same value, and therefore evaluate to the same >> key) and I worry that there is a possibility of the two variables >> taking on different values and breaking the hash. What guarantees >> exist that these values will be the same? Are there any safeguards to >> prevent future patches from accidentally sidestepping these >> guarantees? > > Ah, OK, so this is more about patch 8 than patch 6. So far audit uses inode > pointer as a key - now with patch 8, there is a fsnotify_mark_list attached > to an inode if and only if there is any fsnotify_mark for that inode and > both inode->i_fsnotify_marks (used as a key in inode_to_key()) and > mark->obj_list_head (used as a key in chunk_to_key()) point to it. So keys > for an inode and chunk match if and only if the fsnotify mark in the chunk > is attached to the inode - the same as before patch 8. > > The only reason why I changed audit to use a different pointer for the key > is that you need some lock protection to do mark->obj_list_head->inode > dereference and this seemed the easiest. Actually now that all the lifetime > rules have worked out, I can see we can actually use inode pointer as a key > relatively easily since mark->obj_list_head is stable once you hold a mark > reference so locking would be only intermediate step until this gets > established in the series. Would you prefer me to do that? Unless you can think of any reason why that would be dangerous, I think it would be more obvious and easier to maintain as a result.
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 156b6a93f4fc..f0859828de09 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -163,33 +163,48 @@ enum {HASH_SIZE = 128}; static struct list_head chunk_hash_heads[HASH_SIZE]; static __cacheline_aligned_in_smp DEFINE_SPINLOCK(hash_lock); -static inline struct list_head *chunk_hash(const struct inode *inode) +/* Function to return search key in our hash from inode. */ +static unsigned long inode_to_key(const struct inode *inode) { - unsigned long n = (unsigned long)inode / L1_CACHE_BYTES; + return (unsigned long)inode; +} + +/* + * Function to return search key in our hash from chunk. Key 0 is special and + * should never be present in the hash. + */ +static unsigned long chunk_to_key(struct audit_chunk *chunk) +{ + return (unsigned long)chunk->mark.inode; +} + +static inline struct list_head *chunk_hash(unsigned long key) +{ + unsigned long n = key / L1_CACHE_BYTES; return chunk_hash_heads + n % HASH_SIZE; } /* hash_lock & entry->lock is held by caller */ static void insert_hash(struct audit_chunk *chunk) { - struct fsnotify_mark *entry = &chunk->mark; + unsigned long key = chunk_to_key(chunk); struct list_head *list; - if (!entry->inode) + if (!key) return; - list = chunk_hash(entry->inode); + list = chunk_hash(key); list_add_rcu(&chunk->hash, list); } /* called under rcu_read_lock */ struct audit_chunk *audit_tree_lookup(const struct inode *inode) { - struct list_head *list = chunk_hash(inode); + unsigned long key = inode_to_key(inode); + struct list_head *list = chunk_hash(key); struct audit_chunk *p; list_for_each_entry_rcu(p, list, hash) { - /* mark.inode may have gone NULL, but who cares? */ - if (p->mark.inode == inode) { + if (chunk_to_key(p) == key) { atomic_long_inc(&p->refs); return p; } @@ -585,7 +600,8 @@ int audit_remove_tree_rule(struct audit_krule *rule) static int compare_root(struct vfsmount *mnt, void *arg) { - return d_backing_inode(mnt->mnt_root) == arg; + return inode_to_key(d_backing_inode(mnt->mnt_root)) == + (unsigned long)arg; } void audit_trim_trees(void) @@ -620,9 +636,10 @@ void audit_trim_trees(void) list_for_each_entry(node, &tree->chunks, list) { struct audit_chunk *chunk = find_chunk(node); /* this could be NULL if the watch is dying else where... */ - struct inode *inode = chunk->mark.inode; node->index |= 1U<<31; - if (iterate_mounts(compare_root, inode, root_mnt)) + if (iterate_mounts(compare_root, + (void *)chunk_to_key(chunk), + root_mnt)) node->index &= ~(1U<<31); } spin_unlock(&hash_lock);
Audit tree currently uses inode pointer as a key into the hash table. Getting that from notification mark will be somewhat more difficult with coming fsnotify changes and there's no reason we really have to use the inode pointer. So abstract getting of hash key from the audit chunk and inode so that we can switch to a different key easily later. CC: Paul Moore <paul@paul-moore.com> Signed-off-by: Jan Kara <jack@suse.cz> --- kernel/audit_tree.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)