From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 06/22] audit: Abstract hash key handling Date: Tue, 3 Jan 2017 18:34:19 +0100 Message-ID: <20170103173419.GE3780@quack2.suse.cz> References: <20161222091538.28702-1-jack@suse.cz> <20161222091538.28702-7-jack@suse.cz> <20161223132750.GC22679@quack2.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 Fri 23-12-16 09:13:55, Paul Moore wrote: > On Fri, Dec 23, 2016 at 8:27 AM, Jan Kara wrote: > > 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. > > 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 SUSE Labs, CR