audit.47 filesystem problems (reported by Rob Myers)

[Timothy R. Chavez]

On Tuesday 24 May 2005 14:05, Timothy R. Chavez wrote:
> On Tuesday 24 May 2005 12:43, Timothy R. Chavez wrote:
> 
> <snip>
> 
> > Problem 2:
> > 
> > An OOPS when referencing a NULL pointer when watching /var/audit/audit.log in 
> > an SMP environment which has not yet been reproduced.
> > 
> > Diagnosis:
> > 
> > Because this has not be reproducible and we're in an SMP environment its 
> > probably safe to say there is a race.  Because the OOPs occurred from within 
> > permission() and we're dealing with file system watches it is safe to assume 
> > that the race occurred somewhere in or under audit_notify_watch().
> > 
> > Solution:
> > 
> 
> Ok, just looking at audit_notify_watch() in kernel:auditsc.c,
> 
> int audit_notify_watch(struct inode *inode, int mask)
> {
>         int ret = 0;
>         struct audit_context *context = current->audit_context;
>         struct audit_aux_data_watched *ax;
>         struct audit_wentry *wentry = NULL;
>                                                                                                                                                                      
>         if (likely(!audit_enabled))
>                 goto audit_notify_watch_exit;
>                                                                                                                                                                      
>         if (!inode)
>                 goto audit_notify_watch_exit;
>                                                                                                                                                                      
> Possibly in an SMP environment there could be a race here
> where we're trying to get a reference to inode->i_audit->wentry,
> we pass the check in audit_wentry_get() to check if it's still OK,
> then another CPU changes it, such that it becomes invalid in
> some way (ie: NULL).  With the read_lock() we force anything
> that's interested in this i_audit->wentry to block until we got
> the reference successfully.  Does this make sense?
> 
>         read_lock(&inode->i_audit->lock);
>         wentry = audit_wentry_get(inode->i_audit->wentry);
>         if (!wentry) 
>                 goto audit_notify_watch_exit;
>         read_unlock(&inode->i_audit->lock);
> 

*cough* of course I'd want to release the lock of !wentry :-) *cough*
 
-tim