Re: [PATCH 06/22] audit: Abstract hash key handling

On Fri 23-12-16 09:13:55, Paul Moore wrote: > On Fri, Dec 23, 2016 at 8:27 AM, Jan Kara <jack(a)suse.cz&gt; wrote: > > On Thu 22-12-16 18:27:40, Paul Moore wrote: > >> On Thu, Dec 22, 2016 at 4:15 AM, Jan Kara <jack(a)suse.cz&gt; 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(a)paul-moore.com&gt; > >> > Signed-off-by: Jan Kara <jack(a)suse.cz&gt; > >> > --- > >> > 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?