From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:34300 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932535AbeGDI2m (ORCPT ); Wed, 4 Jul 2018 04:28:42 -0400 Date: Wed, 4 Jul 2018 10:28:40 +0200 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , Linux Audit , Paul Moore , linux-fsdevel , Al Viro , Richard Guy Briggs Subject: Re: [PATCH 3/6] audit: Fix possible tagging failures Message-ID: <20180704082840.4oxvnnoi35t72ko4@quack2.suse.cz> References: <20180628164014.4925-1-jack@suse.cz> <20180628164014.4925-4-jack@suse.cz> <20180703142129.su726stbrjebuwjo@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue 03-07-18 20:42:26, Amir Goldstein wrote: > On Tue, Jul 3, 2018 at 5:21 PM, Jan Kara wrote: > > On Fri 29-06-18 15:05:07, Amir Goldstein wrote: > >> On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara wrote: > >> > Audit tree code is replacing marks attached to inodes in non-atomic way. > >> > Thus fsnotify_find_mark() in tag_chunk() may find a mark that belongs to > >> > a chunk that is no longer valid one and will soon be destroyed. Tags > >> > added to such chunk will be simply lost. > >> > > >> > Fix the problem by making sure old mark is marked as going away (through > >> > fsnotify_detach_mark()) before dropping mark_mutex and thus in an atomic > >> > way wrt tag_chunk(). Note that this does not fix the problem completely > >> > as if tag_chunk() finds a mark that is going away, it fails with > >> > -ENOENT. But at least the failure is not silent and currently there's no > >> > way to search for another fsnotify mark attached to the inode. We'll fix > >> > this problem in later patch. > >> > > >> > Signed-off-by: Jan Kara > >> > --- > >> > >> This one too looks sane. > >> Without knowing anything about audit_watch, there seems to be > >> an fsnotify_destroy_mark() after unlock of audit_filter_mutex, so it > >> may require a similar fix. > > > > Where? I don't see any call to fsnotify_destroy_mark() left after this > > patch... > > > > Not directly related to this cleanup, but looking in other audit modules, > fsnotify_destroy_mark() in audit_remove_parent_watches() is called > outside audit_filter_mutex and audit_find_parent() in audit_add_watch() > is called inside audit_filter_mutex, so I was wondering if there was a > similar race window in that code. I didn't spend enough time looking > at audit_watch.c to understand what is going on in there. Oh, those are completely different fsnotify marks :) They belong to audit_watch_group (while these to audit_tree_group) and these patches have no ambition to change anything there. Honza -- Jan Kara SUSE Labs, CR