From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 06/22] audit: Abstract hash key handling Date: Fri, 23 Dec 2016 14:27:50 +0100 Message-ID: <20161223132750.GC22679@quack2.suse.cz> References: <20161222091538.28702-1-jack@suse.cz> <20161222091538.28702-7-jack@suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org To: Paul Moore Cc: Jan Kara , linux-fsdevel@vger.kernel.org, Amir Goldstein , Lino Sanfilippo , Miklos Szeredi , linux-audit@redhat.com List-Id: linux-audit@redhat.com On Thu 22-12-16 18:27:40, Paul Moore wrote: > On Thu, Dec 22, 2016 at 4:15 AM, Jan Kara 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 > > Signed-off-by: Jan Kara > > --- > > 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 -- Jan Kara SUSE Labs, CR