On Tue, Jan 3, 2017 at 12:34 PM, Jan Kara <jack@xxxxxxx> wrote: > On Fri 23-12-16 09:13:55, Paul Moore wrote: >> On Fri, Dec 23, 2016 at 8:27 AM, Jan Kara <jack@xxxxxxx> wrote: >> > On Thu 22-12-16 18:27:40, Paul Moore wrote: >> >> On Thu, Dec 22, 2016 at 4:15 AM, Jan Kara <jack@xxxxxxx> 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@xxxxxxxxxxxxxx> >> >> > Signed-off-by: Jan Kara <jack@xxxxxxx> >> >> > --- >> >> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
