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>
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> 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>
>> > Signed-off-by: Jan Kara <jack(a)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(a)suse.com>
SUSE Labs, CR