audit: file system auditing based on location and name

To implement this feature we rely on the concepts of a "watch" and "watch list". Directories hold lists of "watches" (ie: "watch lists") that describe auditable file names one level beneath them. If a file holds a pointer into a "watch list" it is auditable. When accessed by a system call, information about the inode and its "watches" is added to the audit context of the current task (an inode may have multiple "watches" if a hard link to a "watched" file is itself being "watched") which is sent to user space upon system call exit.

This is similar to Inotify in that the audit subsystem watches for file system activity and collects information about inodes its interested in, but this is where the similarities stop. Despite the fact that the Inotify requirements only dictate a subset of the activity the audit subsystem is interested in, there is a more fundamental divergence between the two projects. Like audit, Inotify takes paths and resolves them to a single inode. But, unlike audit, Inotify does not find the path itself interesting.

Much like the (device,inode)-based system call filters currently available in the audit subsystem, Inotify targets only individual inodes. Thus, if the underlying inode associated with the file /etc/shadow was changed, and /etc/shadow was being "watched", we'd lose auditability on /etc/shadow across transactions.

More so, Inotify cannot watch inodes that do not yet exist (because the file does not yet exist). To do this, the audit subsystem must hook deeper than Inotify (in fs/dcache.c) to adapt with the file system as it changes. Where it makes sense, the small set of notification hooks in the VFS that Inotify and audit could share should be consolidated.

As inotify works off of open file descriptors, yes, this is true.  But, again, if you think this is really important, then why not just work with inotify to provide that kind of support to it?

I suggest you work together with the inotify developers to hash out your differences, as it sounds like you are duplicating a lot of the same functionality.

Do you have any documetation or example userspace code that shows how to use this auditfs interface you have created?

Some notable implementation details are as follows:

* struct inode is _not_ extended to associate audit data with the inode. A hash table is used in which the inode is hashed to retrieve its audit data. We know if an inode has audit data if I_AUDIT has been turned on in inode->i_state.

* Inodes with audit data are implicitly pinned in memory when I_AUDIT is turned on in inode->i_state. This prevents an auditable incore inode from being pushed out of the icache, preserving auditability even under memory pressures.

why is this? It would be a very logical thing to store this stuff inside the inode. It sounds like a bad design to keep per inode data out of the inode. (if you're concerned about taking a lot of space, put a pointer to a kmalloc()'d piece of memory into the inode instead). A hash is just, well, odd for this.

Quoted 8 lines · Timothy R. Chavez

This is similar to Inotify in that the audit subsystem watches for file system activity and collects information about inodes its interested in, but this is where the similarities stop. Despite the fact that the Inotify requirements only dictate a subset of the activity the audit subsystem is interested in, there is a more fundamental divergence between the two projects. Like audit, Inotify takes paths and resolves them to a single inode. But, unlike audit, Inotify does not find the path itself interesting.

Huh? inotify users find that path interesting, as they use it to act apon.

Quoted 6 lines

Much like the (device,inode)-based system call filters currently available in the audit subsystem, Inotify targets only individual inodes. Thus, if the underlying inode associated with the file /etc/shadow was changed, and /etc/shadow was being "watched", we'd lose auditability on /etc/shadow across transactions.

That's why you watch /etc/ instead, which catches that rename. That being said, why would not inotify also want this functionality if you think it is important?

Quoted 7 lines

More so, Inotify cannot watch inodes that do not yet exist (because the file does not yet exist). To do this, the audit subsystem must hook deeper than Inotify (in fs/dcache.c) to adapt with the file system as it changes. Where it makes sense, the small set of notification hooks in the VFS that Inotify and audit could share should be consolidated.

As inotify works off of open file descriptors, yes, this is true. But, again, if you think this is really important, then why not just work with inotify to provide that kind of support to it?

I suggest you work together with the inotify developers to hash out your differences, as it sounds like you are duplicating a lot of the same functionality.

Also, inotify handles the namespace issues of processes by working off of a file descriptor. How do you handle this?

Do you have any documetation or example userspace code that shows how to use this auditfs interface you have created?

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · Greg KH

Quoted 8 lines · Timothy R. Chavez

This is similar to Inotify in that the audit subsystem watches for file system activity and collects information about inodes its interested in, but this is where the similarities stop. Despite the fact that the Inotify requirements only dictate a subset of the activity the audit subsystem is interested in, there is a more fundamental divergence between the two projects. Like audit, Inotify takes paths and resolves them to a single inode. But, unlike audit, Inotify does not find the path itself interesting.

Quoted 2 lines

Huh? inotify users find that path interesting, as they use it to act apon.

I really didn't want to go back and forth on this (my fingers are still sore from our convo on ltc-interlock :-)), but I might as well oblige.

Quoted 6 lines

Quoted 5 lines

Much like the (device,inode)-based system call filters currently available in the audit subsystem, Inotify targets only individual inodes. Thus, if the underlying inode associated with the file /etc/shadow was changed, and /etc/shadow was being "watched", we'd lose auditability on /etc/shadow across transactions.

Quoted 3 lines

That's why you watch /etc/ instead, which catches that rename. That being said, why would not inotify also want this functionality if you think it is important?

Sure, by watching a directory with Inotify, you receive notification upon rename() events. Now, as far as Inotify is concerned, how this is finally interpreted is up to the user space programs it's feeding.

In terms of audit, there must never be a chance for subversion.

If this feature were implemented in terms of Inotify as it stands now, the processing we do in fs/dcache.c would need to be shifted to a user space app which is obviously out of the question.

One alternative, the one I believe you're in favor of, is to augment Inotify itself.

But, AFAICT, nothing in Inotify's documentation leads me to believe that this adds anything to Inotify itself (with the exception of complication and obfuscation), especially in terms of event notification.

IMO, by tieing our logic to Inotify we'd inadvertently be dooming it to the dreaded Second-system syndrome:

http://en.wikipedia.org/wiki/Second-system_effect

Quoted 7 lines

Quoted 6 lines

More so, Inotify cannot watch inodes that do not yet exist (because the file does not yet exist). To do this, the audit subsystem must hook deeper than Inotify (in fs/dcache.c) to adapt with the file system as it changes. Where it makes sense, the small set of notification hooks in the VFS that Inotify and audit could share should be consolidated.

Quoted 3 lines

As inotify works off of open file descriptors, yes, this is true. But, again, if you think this is really important, then why not just work with inotify to provide that kind of support to it?

I've yet to be convinced that merging these two projects or implementing one in terms of the other has any real benefit to either project in terms of their individual goals and requirements.

The only real similarty between the two projects from my POV is that they are both interested in reporting a subset of file system activity and could benefit from a set of common hooks (ie: fsnotify) where it makes sense.

Quoted 3 lines

I suggest you work together with the inotify developers to hash out your differences, as it sounds like you are duplicating a lot of the same functionality.

Where functionality is the same or similar, we can address that, but surely in the interim the inclusion of this patch into -mm would be beneficial to the 98% of dissimilar functionality :)

Quoted 2 lines

Also, inotify handles the namespace issues of processes by working off of a file descriptor. How do you handle this?

Well I guess this boils down to what you consider an issue.

In terms of audit and this patch, tagging audit info to the inode ensures that it's auditable from any namespace.

When a location and name is audited, the administrator does so from what they consider a meaningful namespace and device (ie: for most, the path /etc/shadow is meaningful, so by "watching" it, you've made a file containing security secrets auditable and not your favorite LOTR ascii art) .

Quoted 3 lines · Greg KH

As inotify works off of open file descriptors, yes, this is true. ?But, again, if you think this is really important, then why not just work with inotify to provide that kind of support to it?

http://marc.theaimsgroup.com/?l=linux-kernel&m=110265021327578&w=2

I think Tim was told not to dig into inotify.

A lot of effort has been put into testing the code Tim has presented with review from several kernel developers (listed in the cc). They too should step up and give their opinion on this.

I want to believe questions were asked about this last December when we were starting into this effort. I think the conclusion from the inotify people was for us to proceed and then when we know what we really want, we can refactor should anything be in common.

Quoted 3 lines

I suggest you work together with the inotify developers to hash out your differences, as it sounds like you are duplicating a lot of the same functionality.

Maybe yes and no. Now that the fs audit code is out, I think we can spot commonality. The only common piece that I can think of is just the hook.

The whole rest of it is different. I hope the inotify people comment on this to see if there is indeed something that should be refactored.

Quoted 2 lines

Do you have any documetation or example userspace code that shows how to use this auditfs interface you have created?

people.redhat.com/sgrubb/audit

The audit package is currently distributed in Fedora Core 4. The code to use Tim's fs audit code is in the user space app, but is waiting for the kernel pieces.

Yes, and then I change namespaces to put /etc/shadow at /foo/baz/etc/shadow and then access it that way? Will the current audit system fail to catch that access?

Quoted 3 lines · Greg KH

Yes, and then I change namespaces to put /etc/shadow at /foo/baz/etc/shadow and then access it that way? Will the current audit system fail to catch that access?

The watch is attached to the inode which you happened to call '/etc' in your namespace, and takes effect in _any_ namespace regardless of the path to it.

In the audit trail, you see the path which was used in the audited process's namespace, and also the filter key which was associated with that watch when you added it.

I fail to see any refactoring here, why not make your patch rely on theirs?

Quoted 2 lines

The whole rest of it is different. I hope the inotify people comment on this to see if there is indeed something that should be refactored.

I realize your userspace access is different, yet I do not believe yet that it should be this way.

No documentation on the auditfs interface :(

Quoted 3 lines

The audit package is currently distributed in Fedora Core 4. The code to use Tim's fs audit code is in the user space app, but is waiting for the kernel pieces.

So the userspace package in FC4 will not use auditfs?

Quoted 2 lines · Greg KH

I fail to see any refactoring here, why not make your patch rely on theirs?

At the time this code was developed, inotify was not in the kernel. We would be patching against another patch that's not in the kernel.

Quoted 5 lines

Quoted 2 lines

The whole rest of it is different. I hope the inotify people comment on this to see if there is indeed something that should be refactored.

I realize your userspace access is different, yet I do not believe yet that it should be this way.

The problems that we are faced with are dictated by CAPP and other security profiles. The audit user space access has been in the kernel for over a year, so that's not really a good thing to go changing.

Quoted 1 line

No documentation on the auditfs interface :(

That's what Tim's email was providing. Its a new component.

Quoted 5 lines

Quoted 3 lines

The audit package is currently distributed in Fedora Core 4. The code to use Tim's fs audit code is in the user space app, but is waiting for the kernel pieces.

So the userspace package in FC4 will not use auditfs?

Right. You get a few warnings due to missing functionality. If the kernel were patched with Tim's code, it all works as expected. We have worked out the user space access and that shouldn't be changing.

You are adding auditfs, a new userspace access, right?

His email provided no documentation that I could see. Am I just missing something?

Quoted 5 lines

Quoted 1 line

So the userspace package in FC4 will not use auditfs?

Right. You get a few warnings due to missing functionality. If the kernel were patched with Tim's code, it all works as expected. We have worked out the user space access and that shouldn't be changing.

Then what use is auditfs for if you don't need it?

Am I correct in thinking that you all need to split this patch into two pieces, the new inode stuff, and auditfs, as neither one has anything to do with the other?

Quoted 14 lines · Timothy R. Chavez

Quoted 12 lines · Greg KH

Quoted 8 lines · Timothy R. Chavez

This is similar to Inotify in that the audit subsystem watches for file system activity and collects information about inodes its interested in, but this is where the similarities stop. Despite the fact that the Inotify requirements only dictate a subset of the activity the audit subsystem is interested in, there is a more fundamental divergence between the two projects. Like audit, Inotify takes paths and resolves them to a single inode. But, unlike audit, Inotify does not find the path itself interesting.

Huh? inotify users find that path interesting, as they use it to act apon.

Quoted 2 lines

I really didn't want to go back and forth on this (my fingers are still sore from our convo on ltc-interlock :-)), but I might as well oblige.

I'm sorry, but discussions on internal mailing lists mean nothing here on lkml. How would we know what you all discussed, or if it was even correct?

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Much like the (device,inode)-based system call filters currently available in the audit subsystem, Inotify targets only individual inodes. Thus, if the underlying inode associated with the file /etc/shadow was changed, and /etc/shadow was being "watched", we'd lose auditability on /etc/shadow across transactions.

Quoted 7 lines

Quoted 3 lines

That's why you watch /etc/ instead, which catches that rename. That being said, why would not inotify also want this functionality if you think it is important?

Sure, by watching a directory with Inotify, you receive notification upon rename() events. Now, as far as Inotify is concerned, how this is finally interpreted is up to the user space programs it's feeding.

Which should be the same as audit, right? audit is just reporting what happens.

Quoted 1 line

In terms of audit, there must never be a chance for subversion.

"never" is a tough thing to prove :)

Quoted 4 lines

If this feature were implemented in terms of Inotify as it stands now, the processing we do in fs/dcache.c would need to be shifted to a user space app which is obviously out of the question.

Why "obviously"? It isn't obvious to me.

Quoted 3 lines

One alternative, the one I believe you're in favor of, is to augment Inotify itself.

Yes.

Quoted 4 lines

But, AFAICT, nothing in Inotify's documentation leads me to believe that this adds anything to Inotify itself (with the exception of complication and obfuscation), especially in terms of event notification.

It would allow inotify to work for you also. That's a good enough reason.

Quoted 4 lines

IMO, by tieing our logic to Inotify we'd inadvertently be dooming it to the dreaded Second-system syndrome:

http://en.wikipedia.org/wiki/Second-system_effect

I fail to see how that has any relevance on this discussion.

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

More so, Inotify cannot watch inodes that do not yet exist (because the file does not yet exist). To do this, the audit subsystem must hook deeper than Inotify (in fs/dcache.c) to adapt with the file system as it changes. Where it makes sense, the small set of notification hooks in the VFS that Inotify and audit could share should be consolidated.

As inotify works off of open file descriptors, yes, this is true. But, again, if you think this is really important, then why not just work with inotify to provide that kind of support to it?

Quoted 3 lines

I've yet to be convinced that merging these two projects or implementing one in terms of the other has any real benefit to either project in terms of their individual goals and requirements.

You have common hooks in the kernel. You do almost the same thing (report fs changes to userspace.) Seems a natural thing to me.

Quoted 4 lines

The only real similarty between the two projects from my POV is that they are both interested in reporting a subset of file system activity and could benefit from a set of common hooks (ie: fsnotify) where it makes sense.

Exactly. What else is there?

Quoted 8 lines

Quoted 4 lines

I suggest you work together with the inotify developers to hash out your differences, as it sounds like you are duplicating a lot of the same functionality.

Where functionality is the same or similar, we can address that, but surely in the interim the inclusion of this patch into -mm would be beneficial to the 98% of dissimilar functionality :)

I don't see such a huge dissimilar functionality, but hey, feel free to prove me wrong :)

Quoted 5 lines

Quoted 3 lines

Also, inotify handles the namespace issues of processes by working off of a file descriptor. How do you handle this?

Well I guess this boils down to what you consider an issue.

Namespaces are an issue.

Quoted 2 lines

In terms of audit and this patch, tagging audit info to the inode ensures that it's auditable from any namespace.

Agreed. But how do you report that inode info to userspace when you don't know the namespace it was accessed under? Or am I missing something here?

Quoted 5 lines

When a location and name is audited, the administrator does so from what they consider a meaningful namespace and device (ie: for most, the path /etc/shadow is meaningful, so by "watching" it, you've made a file containing security secrets auditable and not your favorite LOTR ascii art) .

Yes, and then I change namespaces to put /etc/shadow at /foo/baz/etc/shadow and then access it that way? Will the current audit system fail to catch that access?

thanks,

greg k-h

Tim's code lets you say I want change notification to this file only. The notification follows the audit format with all relavant pieces of information gathered at the time of the event and serialized with all other events.

Quoted 3 lines · Steve Grubb

Tim's code lets you say I want change notification to this file only. The notification follows the audit format with all relavant pieces of information gathered at the time of the event and serialized with all other events.

well can't you sort of do that based on (selinux) security context of the file already? after all that's part of the inode already. Isn't that finegrained enough?

Quoted 10 lines · Arjan van de Ven

Quoted 3 lines · Steve Grubb

Tim's code lets you say I want change notification to this file only. The notification follows the audit format with all relavant pieces of information gathered at the time of the event and serialized with all other events.

well can't you sort of do that based on (selinux) security context of the file already? after all that's part of the inode already. Isn't that finegrained enough?

Provided you make it that far, yes, SE Linux _could_ be used to provide similar functionality. But, what if you bottom out on a DAC decision?

[foo@liltux /]$ cat /etc/shadow
cat: /etc/shadow: Permission denied

Quoted 3 lines

[foo@liltux /]$ cat /etc/shadow
cat: /etc/shadow: Permission denied

Additionally, the apps would need to either be rewritten to create the files under the audited context, or policy would have to cause all files created by those apps to be under the audited context. Neither one of those options is satisfactory

why not?
If your /etc/shadow has no selinux context you've lost already :0

Quoted 1 line · Greg KH

You are adding auditfs, a new userspace access, right?

Not sure what you mean. This is using the same netlink interface that all the rest of the audit system is using for command and control. Nothing has changed here. What is different is the message I send into the kernel. The audit system dispatches it into Tim's code which in turn sets up the watch.

Quoted 2 lines

His email provided no documentation that I could see. Am I just missing something?

The auditfs code is programmed by filling out the watch_transport structure and sending a AUDIT_WATCH_INS message type. The perms, pathlen, & keylen are all that's filled out. The path & key are stored back to back in the payload section. To delete, you do the same thing and send AUDIT_WATCH_REM message. Yes, this should be added to the documentation.

Tim's code lets you say I want change notification to this file only. The notification follows the audit format with all relavant pieces of information gathered at the time of the event and serialized with all other events.

Quoted 3 lines

Am I correct in thinking that you all need to split this patch into two pieces, the new inode stuff, and auditfs, as neither one has anything to do with the other?

It could be split to make it easier to read, but one's useless without the other.

Even if access control prohibits us from actually seeing the content of /etc/shadow, if we're auditing /etc/shadow, attempts should be logged and not gone unnoticed.

watch /etc/shadow
passwd
<inode changes before execution ends>
cat /etc/shadow (goes unaudited)
<send event to user space>
ln /etc/shadow /home/foo/evil_shadow (goes unaudited) <process in user space>
cat /home/foo/evil_shadow (goes unaudited)
<send back instructions to kernel to watch the new /etc/shadow> cat /etc/shadow (audited)

Hmmm...

Quoted 6 lines

Quoted 3 lines

I've yet to be convinced that merging these two projects or implementing one in terms of the other has any real benefit to either project in terms of their individual goals and requirements.

You have common hooks in the kernel. You do almost the same thing (report fs changes to userspace.) Seems a natural thing to me.

This patch reports "fs changes" to the audit subsystem which then adds them to the audit_context of the current task. Then, at system call exit, the audit_context is sent to user space, effectively logging not only the "fs change", but information regarding the process, system call, path, etc, to give the complete account.

Quoted 5 lines

Quoted 3 lines

The only real similarty between the two projects from my POV is that they are both interested in reporting a subset of file system activity and could benefit from a set of common hooks (ie: fsnotify) where it makes sense.

Exactly. What else is there?

The fact that audit is interested in more activity then Inotify (like permission()),

must strive to never lose events,

interacts with the audit subsystem directly,

must conform to security requirements placed on it by various Protection Profiles (for the time being, CAPP),

defers sending the event record to user space until system call exit,

the inode is relevant only if it's associated with the last component of an audited path (not just the inode targetted initially)...

And I'm sure Robert could point out things that Inotify does that audit doesn't do (and isn't very interested in doing).

Quoted 16 lines · Timothy R. Chavez

Even if access control prohibits us from actually seeing the content of /etc/shadow, if we're auditing /etc/shadow, attempts should be logged and not gone unnoticed.

watch /etc/shadow
passwd
<inode changes before execution ends>
cat /etc/shadow (goes unaudited)
<send event to user space>
ln /etc/shadow /home/foo/evil_shadow (goes unaudited) <process in user space>
cat /home/foo/evil_shadow (goes unaudited)
<send back instructions to kernel to watch the new /etc/shadow> cat /etc/shadow (audited)

Quoted 1 line

Hmmm...

Then you watch the directory /etc/ which would have caught all of this, right? My argument is that wouldn't inotify also want to know this exact same thing?

Quoted 11 lines

Quoted 6 lines

Quoted 3 lines

I've yet to be convinced that merging these two projects or implementing one in terms of the other has any real benefit to either project in terms of their individual goals and requirements.

You have common hooks in the kernel. You do almost the same thing (report fs changes to userspace.) Seems a natural thing to me.

This patch reports "fs changes" to the audit subsystem which then adds them to the audit_context of the current task. Then, at system call exit, the audit_context is sent to user space, effectively logging not only the "fs change", but information regarding the process, system call, path, etc, to give the complete account.

Great, so your userspace notification is different than inotify, that's fine. But what you are trying to do within the kernel is pretty much the same, which is my main point.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

The only real similarty between the two projects from my POV is that they are both interested in reporting a subset of file system activity and could benefit from a set of common hooks (ie: fsnotify) where it makes sense.

Exactly. What else is there?

The fact that audit is interested in more activity then Inotify (like permission()),

Fine, inotify users can just ignore those messages. Or perhaps they will grow to want to see them.

Quoted 1 line

must strive to never lose events,

Why is this not a good goal for inotify to also have?

Quoted 1 line

interacts with the audit subsystem directly,

Ok, a difference.

Quoted 2 lines

must conform to security requirements placed on it by various Protection Profiles (for the time being, CAPP),

And those "security requirements" are what?

Quoted 1 line

defers sending the event record to user space until system call exit,

Again, not a big deal for inotify.

Quoted 2 lines

the inode is relevant only if it's associated with the last component of an audited path (not just the inode targetted initially)...

Again, something that I think inotify would like to also know.

Quoted 2 lines

And I'm sure Robert could point out things that Inotify does that audit doesn't do (and isn't very interested in doing).

Sure, just because you all do some things differently, doesn't mean that you shouldn't use the same core code.

Now I know you don't want to worry about another project's code getting into the kernel tree to depend on your changes, but hey, that's life. Try working with the inotify developers instead of ignoring them and duplicating the same stuff.

Also, you still have not pointed out what auditfs is for and how to use it.

thanks,

greg k-h

fs/attr.c +0 -0

 	if (ia_valid & ATTR_SIZE) {
 		if (attr->ia_size != i_size_read(inode)) {
 			error = vmtruncate(inode, attr->ia_size);

diff -Nurp linux-2.6.13-rc1-mm1/fs/namei.c linux-2.6.13-rc1-mm1~auditfs/fs/namei.c

fs/namei.c +4 -0

@@ -243,6 +243,8 @@ int permission(struct inode *inode, int 
 	}
 
 
+	audit_notify_watch(inode, mask);
+
 	/* Ordinary permission routines do not understand MAY_APPEND. */
 	submask = mask & ~MAY_APPEND;
 	if (inode->i_op && inode->i_op->permission)

@@ -358,6 +360,8 @@ static inline int exec_permission_lite(s
 	if (inode->i_op && inode->i_op->permission)
 		return -EAGAIN;
 
+	audit_notify_watch(inode, MAY_EXEC);
+
 	if (current->fsuid == inode->i_uid)
 		mode >>= 6;
 	else if (in_group_p(inode->i_gid))

fs/namei.c +4 -0

 		fsnotify_create(dir, dentry->d_name.name);
 		security_inode_post_create(dir, dentry, mode);
 	}

@@ -1637,6 +1644,7 @@ int vfs_mknod(struct inode *dir, struct 
 	DQUOT_INIT(dir);
 	error = dir->i_op->mknod(dir, dentry, mode, dev);
 	if (!error) {
+		audit_notify_watch(dentry->d_inode, MAY_WRITE);
 		fsnotify_create(dir, dentry->d_name.name);
 		security_inode_post_mknod(dir, dentry, mode, dev);
 	}

@@ -1710,6 +1718,7 @@ int vfs_mkdir(struct inode *dir, struct 
 	DQUOT_INIT(dir);
 	error = dir->i_op->mkdir(dir, dentry, mode);
 	if (!error) {
+		audit_notify_watch(dentry->d_inode, MAY_WRITE);
 		fsnotify_mkdir(dir, dentry->d_name.name);
 		security_inode_post_mkdir(dir,dentry, mode);
 	}

@@ -1951,6 +1960,7 @@ int vfs_symlink(struct inode *dir, struc
 	DQUOT_INIT(dir);
 	error = dir->i_op->symlink(dir, dentry, oldname);
 	if (!error) {
+		audit_notify_watch(dentry->d_inode, MAY_WRITE);
 		fsnotify_create(dir, dentry->d_name.name);
 		security_inode_post_symlink(dir, dentry, oldname);
 	}

@@ -2024,6 +2034,7 @@ int vfs_link(struct dentry *old_dentry, 
 	error = dir->i_op->link(old_dentry, dir, new_dentry);
 	up(&old_dentry->d_inode->i_sem);
 	if (!error) {
+		audit_notify_watch(new_dentry->d_inode, MAY_WRITE);
 		fsnotify_create(dir, new_dentry->d_name.name);
 		security_inode_post_link(old_dentry, dir, new_dentry);
 	}

fs/namei.c +0 -0

 		security_inode_post_rename(old_dir, old_dentry, new_dir, new_dentry);
 	}
 	if (target)

diff -Nurp linux-2.6.13-rc1-mm1/fs/open.c linux-2.6.13-rc1-mm1~auditfs/fs/open.c

fs/open.c +1 -0

@@ -25,6 +25,7 @@
 #include <linux/pagemap.h>
 #include <linux/syscalls.h>
 #include <linux/rcupdate.h>
+#include <linux/audit.h>
 
 #include <asm/unistd.h>
 

fs/open.c +0 -0

 	error = -EROFS;
 	if (IS_RDONLY(inode))
 		goto out;

fs/attr.c int inode_setattr(struct inode * inode, · line 74

Quoted 6 lines · * Timothy R. Chavez ([email protected]) wrote:

fs/attr.c +2 -0

@@ -69,6 +70,8 @@ int inode_setattr(struct inode * inode,
      unsigned int ia_valid = attr->ia_valid;
      int error = 0;
 
+     audit_notify_watch(inode, MAY_WRITE);
+

Hmm, this looks wrong. It should be in notify_change, since I don't think there's a strict requirement that a fs call inode_setattr, which is just a commonly used helper.

Quoted 27 lines

fs/attr.c +0 -0

      if (ia_valid & ATTR_SIZE) {
              if (attr->ia_size != i_size_read(inode)) {
                      error = vmtruncate(inode, attr->ia_size);

diff -Nurp linux-2.6.13-rc1-mm1/fs/namei.c linux-2.6.13-rc1-mm1~auditfs/fs/namei.c

fs/namei.c 2011-07-05 02:01:36.000000000 -0500 renamed from fs/namei.c 2011-07-05 01:57:30.000000000 -0500 +4 -0

@@ -243,6 +243,8 @@ int permission(struct inode *inode, int
      }
 
 
+     audit_notify_watch(inode, mask);
+
      /* Ordinary permission routines do not understand MAY_APPEND. */
      submask = mask & ~MAY_APPEND;
      if (inode->i_op && inode->i_op->permission)

@@ -358,6 +360,8 @@ static inline int exec_permission_lite(s
      if (inode->i_op && inode->i_op->permission)
              return -EAGAIN;
 
+     audit_notify_watch(inode, MAY_EXEC);
+
      if (current->fsuid == inode->i_uid)
              mode >>= 6;
      else if (in_group_p(inode->i_gid))

@@ -1188,6 +1192,8 @@ static inline int may_delete(struct inod
 

fs/namei.c 2011-07-05 02:01:36.000000000 -0500 static inline int may_delete(struct inod · line 1196

Quoted 4 lines

fs/namei.c 2011-07-05 02:01:36.000000000 -0500 renamed from fs/namei.c 2011-07-05 01:57:30.000000000 -0500 +2 -0

      BUG_ON(victim->d_parent->d_inode != dir);
 
+     audit_notify_watch(victim->d_inode, MAY_WRITE);
+

I forget why you need two separate notifications on delete?

fs/namei.c 2011-07-05 02:01:36.000000000 -0500 static inline int may_delete(struct inod · line 1199

Quoted 4 lines

fs/namei.c 2011-07-05 02:01:36.000000000 -0500 renamed from fs/namei.c 2011-07-05 01:57:30.000000000 -0500 +0 -0

      error = permission(dir,MAY_WRITE | MAY_EXEC, NULL);
      if (error)
              return error;

fs/namei.c 2011-07-05 02:01:36.000000000 -0500 int vfs_create(struct inode *dir, struct · line 1321

Quoted 5 lines

fs/namei.c 2011-07-05 02:01:36.000000000 -0500 renamed from fs/namei.c 2011-07-05 01:57:30.000000000 -0500 +1 -0

@@ -1312,6 +1318,7 @@ int vfs_create(struct inode *dir, struct
      DQUOT_INIT(dir);
      error = dir->i_op->create(dir, dentry, mode, nd);
      if (!error) {
+             audit_notify_watch(dentry->d_inode, MAY_WRITE);

And on create. And, as I've mentioned, these bits could be collapsed with fsnotify hooks. I'll split that off from inotify, perhaps you can work against that...

Quoted 36 lines

fs/namei.c 2011-07-05 02:01:36.000000000 -0500 renamed from fs/namei.c 2011-07-05 01:57:30.000000000 -0500 +4 -0

              fsnotify_create(dir, dentry->d_name.name);
              security_inode_post_create(dir, dentry, mode);
      }

@@ -1637,6 +1644,7 @@ int vfs_mknod(struct inode *dir, struct
      DQUOT_INIT(dir);
      error = dir->i_op->mknod(dir, dentry, mode, dev);
      if (!error) {
+             audit_notify_watch(dentry->d_inode, MAY_WRITE);
              fsnotify_create(dir, dentry->d_name.name);
              security_inode_post_mknod(dir, dentry, mode, dev);
      }

@@ -1710,6 +1718,7 @@ int vfs_mkdir(struct inode *dir, struct
      DQUOT_INIT(dir);
      error = dir->i_op->mkdir(dir, dentry, mode);
      if (!error) {
+             audit_notify_watch(dentry->d_inode, MAY_WRITE);
              fsnotify_mkdir(dir, dentry->d_name.name);
              security_inode_post_mkdir(dir,dentry, mode);
      }

@@ -1951,6 +1960,7 @@ int vfs_symlink(struct inode *dir, struc
      DQUOT_INIT(dir);
      error = dir->i_op->symlink(dir, dentry, oldname);
      if (!error) {
+             audit_notify_watch(dentry->d_inode, MAY_WRITE);
              fsnotify_create(dir, dentry->d_name.name);
              security_inode_post_symlink(dir, dentry, oldname);
      }

@@ -2024,6 +2034,7 @@ int vfs_link(struct dentry *old_dentry,
      error = dir->i_op->link(old_dentry, dir, new_dentry);
      up(&old_dentry->d_inode->i_sem);
      if (!error) {
+             audit_notify_watch(new_dentry->d_inode, MAY_WRITE);
              fsnotify_create(dir, new_dentry->d_name.name);
              security_inode_post_link(old_dentry, dir, new_dentry);
      }

fs/namei.c 2011-07-05 02:01:36.000000000 -0500 static int vfs_rename_dir(struct inode * · line 2161

Quoted 5 lines

fs/namei.c 2011-07-05 02:01:36.000000000 -0500 renamed from fs/namei.c 2011-07-05 01:57:30.000000000 -0500 +1 -0

@@ -2147,6 +2158,7 @@ static int vfs_rename_dir(struct inode *
      }
      if (!error) {
              d_move(old_dentry,new_dentry);
+             audit_notify_watch(old_dentry->d_inode, MAY_WRITE);

Don't you get two identical notifications here (one from permission, and one from here)? And where's the update for the new?

fs/namei.c 2011-07-05 02:01:36.000000000 -0500 static int vfs_rename_other(struct inode · line 2190

Quoted 9 lines

fs/namei.c 2011-07-05 02:01:36.000000000 -0500 renamed from fs/namei.c 2011-07-05 01:57:30.000000000 -0500 +1 -0

              security_inode_post_rename(old_dir, old_dentry,
                                         new_dir, new_dentry);
      }

@@ -2175,6 +2187,7 @@ static int vfs_rename_other(struct inode
              /* The following d_move() should become unconditional */
              if (!(old_dir->i_sb->s_type->fs_flags & FS_ODD_RENAME))
                      d_move(old_dentry, new_dentry);
+             audit_notify_watch(old_dentry->d_inode, MAY_WRITE);

Same here?

Quoted 17 lines

fs/namei.c 2011-07-05 02:01:36.000000000 -0500 renamed from fs/namei.c 2011-07-05 01:57:30.000000000 -0500 +0 -0

              security_inode_post_rename(old_dir, old_dentry, new_dir, new_dentry);
      }
      if (target)

diff -Nurp linux-2.6.13-rc1-mm1/fs/open.c linux-2.6.13-rc1-mm1~auditfs/fs/open.c

fs/open.c 2011-07-05 02:01:36.000000000 -0500 renamed from fs/open.c 2011-07-05 01:57:31.000000000 -0500 +1 -0

@@ -25,6 +25,7 @@
 #include <linux/pagemap.h>
 #include <linux/syscalls.h>
 #include <linux/rcupdate.h>
+#include <linux/audit.h>
 
 #include <asm/unistd.h>
 

@@ -608,6 +609,8 @@ asmlinkage long sys_fchmod(unsigned int
 

fs/open.c 2011-07-05 02:01:36.000000000 -0500 asmlinkage long sys_fchmod(unsigned int · line 613

Quoted 4 lines

fs/open.c 2011-07-05 02:01:36.000000000 -0500 renamed from fs/open.c 2011-07-05 01:57:31.000000000 -0500 +2 -0

      dentry = file->f_dentry;
      inode = dentry->d_inode;
+
+     audit_notify_watch(inode, MAY_WRITE);

looks like you'll get two updates for these, this should be done in notify_change.

fs/open.c 2011-07-05 02:01:36.000000000 -0500 asmlinkage long sys_chmod(const char __u · line 647

Quoted 9 lines

fs/open.c 2011-07-05 02:01:36.000000000 -0500 renamed from fs/open.c 2011-07-05 01:57:31.000000000 -0500 +2 -0

      err = -EROFS;
      if (IS_RDONLY(inode))

@@ -640,6 +643,8 @@ asmlinkage long sys_chmod(const char __u
      if (error)
              goto out;
      inode = nd.dentry->d_inode;
+
+     audit_notify_watch(inode, MAY_WRITE);

ditto

fs/open.c 2011-07-05 02:01:36.000000000 -0500 static int chown_common(struct dentry * · line 682

Quoted 7 lines

fs/open.c 2011-07-05 02:01:36.000000000 -0500 renamed from fs/open.c 2011-07-05 01:57:31.000000000 -0500 +1 -0

      error = -EROFS;
      if (IS_RDONLY(inode))

@@ -674,6 +679,7 @@ static int chown_common(struct dentry *
              printk(KERN_ERR "chown_common: NULL inode\n");
              goto out;
      }
+     audit_notify_watch(inode, MAY_WRITE);

ditto, and wouldn't it be useful to know these are attr writes, not just regular writes? and what about xattrs?

fs/open.c 2011-07-05 02:01:36.000000000 -0500 static int chown_common(struct dentry * · line 685

Quoted 4 lines

fs/open.c 2011-07-05 02:01:36.000000000 -0500 renamed from fs/open.c 2011-07-05 01:57:31.000000000 -0500 +0 -0

      error = -EROFS;
      if (IS_RDONLY(inode))
              goto out;

-

I've chosen not to respond to individual segments because the overall theme is that the right thing to do is to not duplicate functionality.

Your suggestion is to merge the two projects.

In your mind, this benefits both projects.

It will
consolidate the common functionality and enhance Inotify such that it is not only an event notification system for user space, but for other parts of the kernel -- namely audit -- as well. Is this a fair assessment?

I think we can all agree that functionality should not be duplicated. I have conceded that having two seperate hooks side-by-side that have the same purpose should be consolidated. Because this would be generic, all other notification hooks should be available to Inotify (and any other subsystem) as well. Agreed?

My vision is that audit, Inotify, and whatever else plugs into this framework and receives notifications this way -- a file system event notification system for the kernel. Then, audit, Inotify, etc would process this even information it receives and does with it what they will.

Ultimately, the part where we differ most, is the processing of information in fs/dcache.c to give dynamic updates in response to file system activity (such as attaching audit information to an auditable file whose inode just changed). I believe this should be kept seperate and not part of this framework nor Inotify. It's a specific requirement for audit, but not for Inotify. This is one of the places the two systems are functionally different.

By doing it this way, both projects can retain their original goals and meet their individual requirements, and all the common pieces are consolidated in a logical way. This is something Robert and I had discussed on LKML in early December 2004, and was brought up in a discussion more recently for an RFC on LKML in early June between Christoph Hellwig and myself.

Can this patch not be placed in -mm for the time being, as-is? Surely the logic it implements, what I envision will ultimately be retained, could benefit from the exposure in the interim?

Quoted 7 lines

Quoted 6 lines

Ultimately, the part where we differ most, is the processing of information in fs/dcache.c to give dynamic updates in response to file system activity (such as attaching audit information to an auditable file whose inode just changed). I believe this should be kept seperate and not part of this framework nor Inotify. It's a specific requirement for audit, but not for Inotify. This is one of the places the two systems are functionally different.

Quoted 2 lines

I don't think it should be different. If inotify wants to just ignore this information, it can.

Doesn't this mentality bring with it the risk of bloating a framework that should be as "trim" as possible?