[2.6.16-rc5-m3 PATCH] inotify: add the monitor for the event source

[2.6.16-rc5-m3 PATCH] inotify: add the monitor for the event source

[Yi Yang]

Current inotify implementation only focus on change of file system, but it doesn't
 know who results in this change, this patch adds three fields to struct inotify_event,
 tgid, uid and gid, they will save process ID, user ID and user group ID of the process
 which leads to change in the file system, such software as anti-virus can make use 
of this feature to monitor who is modifying a specific file.

Signed-off-by: Yi Yang <yang.y.yi@gmail.com>
--- a/include/linux/inotify.h.orig	2006-03-08 21:40:12.000000000 +0800
+++ b/include/linux/inotify.h	2006-03-08 23:51:54.000000000 +0800
@@ -19,6 +19,9 @@ struct inotify_event {
 	__s32		wd;		/* watch descriptor */
 	__u32		mask;		/* watch mask */
 	__u32		cookie;		/* cookie to synchronize two events */
+	__u32		tgid;		/* process ID of the event source */
+	__u32		uid;		/* user ID of the responding process */
+	__u32		gid;		/* group ID of the responding process */
 	__u32		len;		/* length (including nulls) of name */
 	char		name[0];	/* stub for possible name */
 };
--- a/fs/inotify.c.orig	2006-03-08 20:58:31.000000000 +0800
+++ b/fs/inotify.c	2006-03-09 00:05:47.000000000 +0800
@@ -219,6 +219,9 @@ static struct inotify_kernel_event * ker
 	kevent->event.wd = wd;
 	kevent->event.mask = mask;
 	kevent->event.cookie = cookie;
+	kevent->event.tgid = current->tgid;
+	kevent->event.uid = current->uid;
+	kevent->event.gid = current->gid;
 
 	INIT_LIST_HEAD(&kevent->list);
 

Re: [2.6.16-rc5-m3 PATCH] inotify: add the monitor for the event source

[Arjan van de Ven]

On Thu, 2006-03-09 at 00:33 +0800, Yi Yang wrote:
> Current inotify implementation only focus on change of file system, but it doesn't
>  know who results in this change, this patch adds three fields to struct inotify_event,
>  tgid, uid and gid, they will save process ID, user ID and user group ID of the process
>  which leads to change in the file system, such software as anti-virus can make use 
> of this feature to monitor who is modifying a specific file.


this patch appears to change the ABI! That is bad bad bad.
Also, how can you guarantee that "current" is valid and meaningful at
the place you use it to get the user id ??
Also the process ID part is really bogus, after all the process may have
exited by the time the inotify client gets to it, and the PID may even
already have been reused.

Re: [2.6.16-rc5-m3 PATCH] inotify: add the monitor for the event source

[Yi Yang]

Arjan van de Ven wrote:
> On Thu, 2006-03-09 at 00:33 +0800, Yi Yang wrote:
>   
>> Current inotify implementation only focus on change of file system, but it doesn't
>>  know who results in this change, this patch adds three fields to struct inotify_event,
>>  tgid, uid and gid, they will save process ID, user ID and user group ID of the process
>>  which leads to change in the file system, such software as anti-virus can make use 
>> of this feature to monitor who is modifying a specific file.
>>     
>
>
> this patch appears to change the ABI! That is bad bad bad.
>   
a change of struct inotify_event can't change ABI, can you describe it 
more clear?
> Also, how can you guarantee that "current" is valid and meaningful at
> the place you use it to get the user id ??
>   
Of course, current process/thread never disappears before fsnotify_* 
returns.
> Also the process ID part is really bogus, after all the process may have
> exited by the time the inotify client gets to it, and the PID may even
> already have been reused.
>
>   
Your concern is correct, but uid and git can give out some hints, I ever 
considered to
save the name of current process, however that needs a bigger and 
length-variable
inotify_event struct, moreover, to get the full path name of current 
process/thread
in kernel will have a big overhead, so I must select a comprise way. In 
fact, the case
 you said is very few.

Re: [2.6.16-rc5-m3 PATCH] inotify: add the monitor for the event source

[Arjan van de Ven]

On Thu, 2006-03-09 at 13:18 +0800, Yi Yang wrote:
> Arjan van de Ven wrote:
> > On Thu, 2006-03-09 at 00:33 +0800, Yi Yang wrote:
> >   
> >> Current inotify implementation only focus on change of file system, but it doesn't
> >>  know who results in this change, this patch adds three fields to struct inotify_event,
> >>  tgid, uid and gid, they will save process ID, user ID and user group ID of the process
> >>  which leads to change in the file system, such software as anti-virus can make use 
> >> of this feature to monitor who is modifying a specific file.
> >>     
> >
> >
> > this patch appears to change the ABI! That is bad bad bad.
> >   
> a change of struct inotify_event can't change ABI, can you describe it 
> more clear?

it breaks ABI because this structure is communicated to userspace, and
you change both the layout and the size of it. What else would ABI
mean??


> > Also, how can you guarantee that "current" is valid and meaningful at
> > the place you use it to get the user id ??
> >   
> Of course, current process/thread never disappears before fsnotify_* 
> returns.

but... what makes you think it's not a kernel thread such as kjournald?
(which have basically meaningless current)


> > Also the process ID part is really bogus, after all the process may have
> > exited by the time the inotify client gets to it, and the PID may even
> > already have been reused.
> >
> >   
> Your concern is correct, but uid and git can give out some hints, I ever 
> considered to
> save the name of current process, however that needs a bigger and 
> length-variable
> inotify_event struct, moreover, to get the full path name of current 
> process/thread
> in kernel will have a big overhead, so I must select a comprise way.

there is no "full path name" concept in linux like that. And even worse,
many processes will not have *any* path because they have been deleted,
especially the viruses will use this ;)

Re: [2.6.16-rc5-m3 PATCH] inotify: add the monitor for the event source

[Yi Yang]

Arjan van de Ven wrote:
> On Thu, 2006-03-09 at 13:18 +0800, Yi Yang wrote:
>   
>> Arjan van de Ven wrote:
>>     
>>> On Thu, 2006-03-09 at 00:33 +0800, Yi Yang wrote:
>>>   
>>>       
>>>> Current inotify implementation only focus on change of file system, but it doesn't
>>>>  know who results in this change, this patch adds three fields to struct inotify_event,
>>>>  tgid, uid and gid, they will save process ID, user ID and user group ID of the process
>>>>  which leads to change in the file system, such software as anti-virus can make use 
>>>> of this feature to monitor who is modifying a specific file.
>>>>     
>>>>         
>>> this patch appears to change the ABI! That is bad bad bad.
>>>   
>>>       
>> a change of struct inotify_event can't change ABI, can you describe it 
>> more clear?
>>     
>
> it breaks ABI because this structure is communicated to userspace, and
> you change both the layout and the size of it. What else would ABI
> mean??
>   
Many structures exported to user space in kernel  are undergoing some 
change, A good application shouldn't count on invariability forever,
My test application hasn't any problem before change and after change.

>
>   
>>> Also, how can you guarantee that "current" is valid and meaningful at
>>> the place you use it to get the user id ??
>>>   
>>>       
>> Of course, current process/thread never disappears before fsnotify_* 
>> returns.
>>     
>
> but... what makes you think it's not a kernel thread such as kjournald?
> (which have basically meaningless current)
>   
you can get  values of these fields without any problem for kernel 
thread although they are useless.
>
>   
>>> Also the process ID part is really bogus, after all the process may have
>>> exited by the time the inotify client gets to it, and the PID may even
>>> already have been reused.
>>>
>>>   
>>>       
>> Your concern is correct, but uid and git can give out some hints, I ever 
>> considered to
>> save the name of current process, however that needs a bigger and 
>> length-variable
>> inotify_event struct, moreover, to get the full path name of current 
>> process/thread
>> in kernel will have a big overhead, so I must select a comprise way.
>>     
>
> there is no "full path name" concept in linux like that. And even worse,
> many processes will not have *any* path because they have been deleted,
> especially the viruses will use this ;)
>   
For this case you said, this patch has now way really, do you have a 
good way to handle this case?
>
>
>   

Re: [2.6.16-rc5-m3 PATCH] inotify: add the monitor for the event source

[Arjan van de Ven]

> >
> > it breaks ABI because this structure is communicated to userspace, and
> > you change both the layout and the size of it. What else would ABI
> > mean??
> >   
> Many structures exported to user space in kernel  are undergoing some 
> change, A good application shouldn't count on invariability forever,
> My test application hasn't any problem before change and after change.


this is absolutely incorrect. This is an ABI that cannot change in any
incompatible way.
> >
> > but... what makes you think it's not a kernel thread such as kjournald?
> > (which have basically meaningless current)
> >   
> you can get  values of these fields without any problem for kernel 
> thread although they are useless.

exactly

> >
> > there is no "full path name" concept in linux like that. And even worse,
> > many processes will not have *any* path because they have been deleted,
> > especially the viruses will use this ;)
> >   
> For this case you said, this patch has now way really, do you have a 
> good way to handle this case?

it sounds that what you want to achieve is broken in the first place...
(or should use audit etc)

Re: [2.6.16-rc5-m3 PATCH] inotify: add the monitor for the event source

[Yi Yang]

Arjan van de Ven wrote:
>>> it breaks ABI because this structure is communicated to userspace, and
>>> you change both the layout and the size of it. What else would ABI
>>> mean??
>>>   
>>>       
>> Many structures exported to user space in kernel  are undergoing some 
>> change, A good application shouldn't count on invariability forever,
>> My test application hasn't any problem before change and after change.
>>     
>
>
> this is absolutely incorrect. This is an ABI that cannot change in any
> incompatible way.
>   
>>> but... what makes you think it's not a kernel thread such as kjournald?
>>> (which have basically meaningless current)
>>>   
>>>       
>> you can get  values of these fields without any problem for kernel 
>> thread although they are useless.
>>     
>
> exactly
>
>   
>>> there is no "full path name" concept in linux like that. And even worse,
>>> many processes will not have *any* path because they have been deleted,
>>> especially the viruses will use this ;)
>>>   
>>>       
>> For this case you said, this patch has now way really, do you have a 
>> good way to handle this case?
>>     
>
> it sounds that what you want to achieve is broken in the first place...
> (or should use audit etc)
>   
As I known, BSD process audit only can be done inside a process, and 
audit result is just visible after
termination of this process, if an application wants to monitor all the 
processes, it has no way. My patch
 provides such a way bases on inotify with minimal work, it should be an 
good extension for
inotify although it can't cover all the cases.
>
>   

Re: [2.6.16-rc5-m3 PATCH] inotify: add the monitor for the event source

[Arjan van de Ven]

> >>>       
> >> For this case you said, this patch has now way really, do you have a 
> >> good way to handle this case?
> >>     
> >
> > it sounds that what you want to achieve is broken in the first place...
> > (or should use audit etc)
> >   
> As I known, BSD process audit only

I'm not talking about BSD audit but about the CAPP security audit
framework.