| Message ID | 20170403153424.24945-6-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 3, 2017 at 11:33 AM, Jan Kara <jack@suse.cz> wrote: > Currently audit code uses checking of mark->inode to verify whether mark > is still alive. Switch that to checking mark flags as that is more > logical and current way will become unreliable in future. > > Reviewed-by: Miklos Szeredi <mszeredi@redhat.com> > Signed-off-by: Jan Kara <jack@suse.cz> > --- > kernel/audit_tree.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Should audit_tree.c:insert_hash() also be updated in a similar manner? > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > index 11c7ac441624..f12bd40fb8f1 100644 > --- a/kernel/audit_tree.c > +++ b/kernel/audit_tree.c > @@ -248,7 +248,7 @@ static void untag_chunk(struct node *p) > > mutex_lock(&entry->group->mark_mutex); > spin_lock(&entry->lock); > - if (chunk->dead || !entry->inode) { > + if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { > spin_unlock(&entry->lock); > mutex_unlock(&entry->group->mark_mutex); > if (new) > @@ -408,7 +408,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) > > mutex_lock(&old_entry->group->mark_mutex); > spin_lock(&old_entry->lock); > - if (!old_entry->inode) { > + if (!(old_entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { > /* old_entry is being shot, lets just lie */ > spin_unlock(&old_entry->lock); > mutex_unlock(&old_entry->group->mark_mutex); > -- > 2.10.2 >
On Tue 04-04-17 16:47:11, Paul Moore wrote: > On Mon, Apr 3, 2017 at 11:33 AM, Jan Kara <jack@suse.cz> wrote: > > Currently audit code uses checking of mark->inode to verify whether mark > > is still alive. Switch that to checking mark flags as that is more > > logical and current way will become unreliable in future. > > > > Reviewed-by: Miklos Szeredi <mszeredi@redhat.com> > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > kernel/audit_tree.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > Should audit_tree.c:insert_hash() also be updated in a similar manner? Do you mean the part which has become chunk_to_key()? The code there is correct as is since we just use the value of inode pointer for hashing but never dereference it. Also some of the callers of chunk_to_key() don't hold locks that would prevent chunk.mark->connector->inode becoming NULL just after we checked the FSNOTIFY_MARK_FLAG_ATTACHED flag so in practice nothing would change. So I've decided to leave that code as is. That being said I don't have a strong opinion about it so if you prefer checking the flag, I can do it that way. Honza > > > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > > index 11c7ac441624..f12bd40fb8f1 100644 > > --- a/kernel/audit_tree.c > > +++ b/kernel/audit_tree.c > > @@ -248,7 +248,7 @@ static void untag_chunk(struct node *p) > > > > mutex_lock(&entry->group->mark_mutex); > > spin_lock(&entry->lock); > > - if (chunk->dead || !entry->inode) { > > + if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { > > spin_unlock(&entry->lock); > > mutex_unlock(&entry->group->mark_mutex); > > if (new) > > @@ -408,7 +408,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) > > > > mutex_lock(&old_entry->group->mark_mutex); > > spin_lock(&old_entry->lock); > > - if (!old_entry->inode) { > > + if (!(old_entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { > > /* old_entry is being shot, lets just lie */ > > spin_unlock(&old_entry->lock); > > mutex_unlock(&old_entry->group->mark_mutex); > > -- > > 2.10.2 > > > > -- > paul moore > www.paul-moore.com
On Wed, Apr 5, 2017 at 3:38 AM, Jan Kara <jack@suse.cz> wrote: > On Tue 04-04-17 16:47:11, Paul Moore wrote: >> On Mon, Apr 3, 2017 at 11:33 AM, Jan Kara <jack@suse.cz> wrote: >> > Currently audit code uses checking of mark->inode to verify whether mark >> > is still alive. Switch that to checking mark flags as that is more >> > logical and current way will become unreliable in future. >> > >> > Reviewed-by: Miklos Szeredi <mszeredi@redhat.com> >> > Signed-off-by: Jan Kara <jack@suse.cz> >> > --- >> > kernel/audit_tree.c | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> >> Should audit_tree.c:insert_hash() also be updated in a similar manner? > > Do you mean the part which has become chunk_to_key()? ... No, I was talking about the if conditional near the top of the function that checks to see if the fsnotify_mark's inode is non-NULL; it seems like you would also want to convert that to a FSNOTIFY_MARK_FLAG_ATTACHED, yes?
On Thu 06-04-17 07:51:48, Paul Moore wrote: > On Wed, Apr 5, 2017 at 3:38 AM, Jan Kara <jack@suse.cz> wrote: > > On Tue 04-04-17 16:47:11, Paul Moore wrote: > >> On Mon, Apr 3, 2017 at 11:33 AM, Jan Kara <jack@suse.cz> wrote: > >> > Currently audit code uses checking of mark->inode to verify whether mark > >> > is still alive. Switch that to checking mark flags as that is more > >> > logical and current way will become unreliable in future. > >> > > >> > Reviewed-by: Miklos Szeredi <mszeredi@redhat.com> > >> > Signed-off-by: Jan Kara <jack@suse.cz> > >> > --- > >> > kernel/audit_tree.c | 4 ++-- > >> > 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> Should audit_tree.c:insert_hash() also be updated in a similar manner? > > > > Do you mean the part which has become chunk_to_key()? ... > > No, I was talking about the if conditional near the top of the > function that checks to see if the fsnotify_mark's inode is non-NULL; > it seems like you would also want to convert that to a > FSNOTIFY_MARK_FLAG_ATTACHED, yes? Ah, that one. Yes, that can certainly become a FSNOTIFY_MARK_FLAG_ATTACHED check although I cannot currently come up with a situation where it would matter. But it looks safer that way so I'll change that check as you suggest. Honza
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 11c7ac441624..f12bd40fb8f1 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -248,7 +248,7 @@ static void untag_chunk(struct node *p) mutex_lock(&entry->group->mark_mutex); spin_lock(&entry->lock); - if (chunk->dead || !entry->inode) { + if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { spin_unlock(&entry->lock); mutex_unlock(&entry->group->mark_mutex); if (new) @@ -408,7 +408,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) mutex_lock(&old_entry->group->mark_mutex); spin_lock(&old_entry->lock); - if (!old_entry->inode) { + if (!(old_entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { /* old_entry is being shot, lets just lie */ spin_unlock(&old_entry->lock); mutex_unlock(&old_entry->group->mark_mutex);