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

[Paul Moore]

On Tue, Jan 3, 2017 at 12:34 PM, Jan Kara <jack at 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 at 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 at 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 at paul-moore.com>
>> >> > Signed-off-by: Jan Kara <jack at 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.

-- 
paul moore
www.paul-moore.com