headers
Lino Sanfilippo | 19 Nov 2009 09:58
Favicon

Bug in mask_proc (repost)


Hello,

This is a repost of a patch that I posted on 9th July 2009. This time 
the patch is
for the recent dazukofs version (3.1.1).

The problem is that there is a (dazukofs_proc) structure allocated  on 
the stack and
afterwards put in a global list. This will very likely leed to problems 
sooner or later,
since the stack memory may be overwritten the next time the proc 
structure is accessed
in the list.

This patch solves this problem by using a structure that is allocated on 
the heap.

Regards,
Lino Sanfilippo

Geschäftsführender Gesellschafter: Tjark Auerbach
Sitz der Gesellschaft: Tettnang
Handelsregister: Amtsgericht Ulm, HRB 630992
ALLGEMEINE GESCHÄFTSBEDINGUNGEN
Es gelten unsere Allgemeinen Geschäftsbedingungen
(AGB). Sie finden sie in der jeweils gültigen Fassung
im Internet unter http://www.avira.de/agb
***************************************************
(Continue reading)






Permalink
| Reply
| 7 comments  
| 



  

  

  





headers


John Ogness
 | 
21 Nov 2009 09:01





Re: Bug in mask_proc (repost)


On 2009-11-19, Lino Sanfilippo <lino.sanfilippo <at> avira.com> wrote:
> This is a repost of a patch that I posted on 9th July 2009. This
> time the patch is for the recent dazukofs version (3.1.1).
>
> The problem is that there is a (dazukofs_proc) structure allocated
> on the stack and afterwards put in a global list. This will very
> likely leed to problems sooner or later, since the stack memory may
> be overwritten the next time the proc structure is accessed in the
> list.

Please specify an example code path in Linux proving the problem can
occur.

In my opinion you are fixing a problem that does not exist and instead
are creating an extra (expensive) malloc for every file access.

John Ogness

--

-- 
Dazuko Maintainer






Permalink
| Reply
| 6 comments  
| 



  

  

  





headers


John Ogness
 | 
21 Nov 2009 20:22





DazukoFS 3.1.2 released


Hi,

An anonymous post to the Dazuko Savannah page [0] pointed out that
there was a reference management problem with the ignore feature
(/dev/dazukofs.ign). To fix this, version 3.1.2 has been released.

John Ogness

[0] https://savannah.nongnu.org/bugs/?28045

--

-- 
Dazuko Maintainer






Permalink
| Reply
| 0 comments  
| 



  

  

  





headers


Lino Sanfilippo
 | 
24 Nov 2009 14:56






Favicon





Re: Bug in mask_proc (repost)


John Ogness wrote:
>
> Please specify an example code path in Linux proving the problem can
> occur.
>   

This problem can occur every time after the daemon that ignored itself
terminates (unexpectedly or not) before it is able to remove its
proc structure from the list.
When this happens there is a proc structure in the list pointing to
the stack memory of a process that does not exist any more.
And who can say what that memory actually contains the next time
it is accessed?
Sooner or later it is reused by the kernel (maybe for a new process),
and its content is modified in an unpredictable way.

The patch I provided before does not handle those obsolete list entries,
too. But it ensures that the content of the memory at which the list
entries point to, cant be modified in an unpredictable way.

But anyway, maybe we can get rid of this whole recursion stuff at all:
Why do we not simply pass the _lower_ mount and the _lower_ dentry
to dentry_open()? I tried it and it seems to work (or do I oversee a
reason why this could lead to problems?)
I created a patch for 3.1.2 (see attachment) , if you agree that this 
could work
we could remove that whole recursion implementation (including the patch
I sent before).

Regards,

(Continue reading)






Permalink
| Reply
| 5 comments  
| 



  

  

  





headers


Lino Sanfilippo
 | 
24 Nov 2009 15:17






Favicon





Re: Bug in mask_proc (repost)


Lino Sanfilippo wrote:
> This problem can occur every time after the daemon that ignored itself
> terminates (unexpectedly or not) before it is able to remove its
> proc structure from the list.

Ok, forget about that. Of course that cant happen, since a signal (like 
SIGTERM)
sent to the process wont keep the kernel code from cleaning up before 
the process
is terminated. So it should always be able to clean up its own list entry :P

Geschäftsführender Gesellschafter: Tjark Auerbach
Sitz der Gesellschaft: Tettnang
Handelsregister: Amtsgericht Ulm, HRB 630992
ALLGEMEINE GESCHÄFTSBEDINGUNGEN
Es gelten unsere Allgemeinen Geschäftsbedingungen
(AGB). Sie finden sie in der jeweils gültigen Fassung
im Internet unter http://www.avira.de/agb
***************************************************






Permalink
| Reply
| 4 comments  
| 



  

  

  





headers


John Ogness
 | 
25 Nov 2009 22:40





Re: Bug in mask_proc (repost)


On 2009-11-24, Lino Sanfilippo <lino.sanfilippo <at> avira.com> wrote:
>> This problem can occur every time after the daemon that ignored
>> itself terminates (unexpectedly or not) before it is able to remove
>> its proc structure from the list.
>
> Ok, forget about that. Of course that cant happen, since a signal
> (like SIGTERM) sent to the process wont keep the kernel code from
> cleaning up before the process is terminated. So it should always be
> able to clean up its own list entry :P

I actually thought a lot about this when I first implemented it. And I
just finished investigating it again. It is assumed that no preemption
occurs in the calling chain between event.c:dentry_open() and
file.c:dazukofs_open(). Right now that assumption is correct, but it
could change at some point in the future.

If preemption did occur, it would be possible for
dazukofs_remove_group() to be called, which could result in
check_access_precheck() aborting without removing the process from the
list (if group_count was now 0). This would then lead to invalid
memory in the list, because open_file() also would not remove it.

Probably a good (and efficient) fix would be to have check_recursion()
set a "removed" flag in the process structure. That way, open_file()
could very easily see if it has been removed or not. In my opinion,
that is much better than relying on the return value of dentry_open().

Rather than:

ec->file = dentry_open(dget(evt->dentry), mntget(evt->mnt),

(Continue reading)






Permalink
| Reply
| 3 comments  
| 



  

  

  





headers


Lino Sanfilippo
 | 
26 Nov 2009 10:28






Favicon





Re: Bug in mask_proc (repost)


John Ogness wrote:
> Probably a good (and efficient) fix would be to have check_recursion()
> set a "removed" flag in the process structure. That way, open_file()
> could very easily see if it has been removed or not. In my opinion,
> that is much better than relying on the return value of dentry_open().
>
> Rather than:
>
> ec->file = dentry_open(dget(evt->dentry), mntget(evt->mnt),
>                        O_RDONLY | O_LARGEFILE, current_cred());
> if (IS_ERR(ec->file)) {
>         check_recursion();  /* remove myself from proc_list */
>         ret = PTR_ERR(ec->file);
>         goto error_out2;
> }
>
> the code could look like this:
>
> ec->file = dentry_open(dget(evt->dentry), mntget(evt->mnt),
>                        O_RDONLY | O_LARGEFILE, current_cred());
> if (!proc.removed)
>         check_recursion();  /* remove myself from proc_list */
> if (IS_ERR(ec->file)) {
>         ret = PTR_ERR(ec->file);
>         goto error_out2;
> }
>
> John Ogness
>
>   

(Continue reading)






Permalink
| Reply
| 1 comment  
| 



  

  

  





headers


John Ogness
 | 
26 Nov 2009 23:21





Re: Bug in mask_proc (repost)


On 2009-11-26, Lino Sanfilippo <lino.sanfilippo <at> avira.com> wrote:
> And what about the solution I suggested to avoid the whole recursion
> thing (by simply passing _lower_ dentry and mount to
> dazukofs_open()).  Do you (or someone else) see any problems with
> that?

I don't like the idea of userspace getting a handle directly to the
lower mount. All activity of the registered process would go unnoticed
by DazukoFS. This may even cause problems because the atime's could
become out of sync.

I'm not certain how safe it is for any processes to be playing with
files underneath DazukoFS while DazukoFS is mounted.

John Ogness

--

-- 
Dazuko Maintainer






Permalink
| Reply
| 0 comments  
| 



  

  

  





headers


Lino Sanfilippo
 | 
27 Nov 2009 15:04






Favicon





Re: Bug in mask_proc (repost)


John Ogness wrote:
> I actually thought a lot about this when I first implemented it. And I
> just finished investigating it again. It is assumed that no preemption
> occurs in the calling chain between event.c:dentry_open() and
> file.c:dazukofs_open(). Right now that assumption is correct...
>   

Why do you think this assumption is correct? If you compile your kernel
with kernel preemption enabled, kernel code in process context may
be preempted at any time (except if you hold a lock or disable preemption
explicitly with preempt_disable()). And think of multiprocessor systems:
The last daemon may be unregistered on one processor while the other
one is still processing a file access.

> Rather than:
>
> ec->file = dentry_open(dget(evt->dentry), mntget(evt->mnt),
>                        O_RDONLY | O_LARGEFILE, current_cred());
> if (IS_ERR(ec->file)) {
>         check_recursion();  /* remove myself from proc_list */
>         ret = PTR_ERR(ec->file);
>         goto error_out2;
> }
>
> the code could look like this:
>
> ec->file = dentry_open(dget(evt->dentry), mntget(evt->mnt),
>                        O_RDONLY | O_LARGEFILE, current_cred());
> if (!proc.removed)
>         check_recursion();  /* remove myself from proc_list */

(Continue reading)






Permalink
| Reply
| 0 comments  
| 



  

  

  

  


  





    



Gmane