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

[Jan Kara <jack@suse.cz>]

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR