Patchwork [4/4] ext4: fix possible use-after-free ext4_remove_li_request()

login
register
mail settings
Submitter Lukas Czerner
Date May 9, 2011, 3:57 p.m.
Message ID <1304956630-20384-4-git-send-email-lczerner@redhat.com>
Download mbox | patch
Permalink /patch/94789/
State Superseded
Headers show

Comments

Lukas Czerner - May 9, 2011, 3:57 p.m.
We need to take reference to the s_li_request after we take a mutex,
because it might be freed since then, hence result in accessing old
already freed memory. Also we should protect the whole
ext4_remove_li_request() because ext4_li_info might be in the process of
being freed in ext4_lazyinit_thread().

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/super.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)
Eric Sandeen - May 19, 2011, 8:05 p.m.
On 5/9/11 10:57 AM, Lukas Czerner wrote:
> We need to take reference to the s_li_request after we take a mutex,
> because it might be freed since then, hence result in accessing old
> already freed memory. Also we should protect the whole
> ext4_remove_li_request() because ext4_li_info might be in the process of
> being freed in ext4_lazyinit_thread().

It'd be really great to have some comments which explain just what
ext4_li_mtx protects, but I'm working on an add-comments patch for
the lazyinit stuff (I commented things a bit as I reviewed your 
changes) so I'll send that along later.

in any case, the change looks ok, thanks.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>


> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  fs/ext4/super.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c379af6..6a8e48f 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2721,14 +2721,16 @@ static void ext4_remove_li_request(struct ext4_li_request *elr)
>  
>  static void ext4_unregister_li_request(struct super_block *sb)
>  {
> -	struct ext4_li_request *elr = EXT4_SB(sb)->s_li_request;
> -
> -	if (!ext4_li_info)
> +	mutex_lock(&ext4_li_mtx);
> +	if (!ext4_li_info) {
> +		mutex_unlock(&ext4_li_mtx);
>  		return;
> +	}
>  
>  	mutex_lock(&ext4_li_info->li_list_mtx);
> -	ext4_remove_li_request(elr);
> +	ext4_remove_li_request(EXT4_SB(sb)->s_li_request);
>  	mutex_unlock(&ext4_li_info->li_list_mtx);
> +	mutex_unlock(&ext4_li_mtx);
>  }
>  
>  static struct task_struct *ext4_lazyinit_task;

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Czerner - May 20, 2011, 9:27 a.m.
On Thu, 19 May 2011, Eric Sandeen wrote:

> On 5/9/11 10:57 AM, Lukas Czerner wrote:
> > We need to take reference to the s_li_request after we take a mutex,
> > because it might be freed since then, hence result in accessing old
> > already freed memory. Also we should protect the whole
> > ext4_remove_li_request() because ext4_li_info might be in the process of
> > being freed in ext4_lazyinit_thread().
> 
> It'd be really great to have some comments which explain just what
> ext4_li_mtx protects, but I'm working on an add-comments patch for
> the lazyinit stuff (I commented things a bit as I reviewed your 
> changes) so I'll send that along later.

The ext4_li_mtx is protecting the whole ext4_li_info structure so it can
be atomically created and freed.

> 
> in any case, the change looks ok, thanks.
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Thanks!
-Lukas
> 
> 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > ---
> >  fs/ext4/super.c |   10 ++++++----
> >  1 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index c379af6..6a8e48f 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -2721,14 +2721,16 @@ static void ext4_remove_li_request(struct ext4_li_request *elr)
> >  
> >  static void ext4_unregister_li_request(struct super_block *sb)
> >  {
> > -	struct ext4_li_request *elr = EXT4_SB(sb)->s_li_request;
> > -
> > -	if (!ext4_li_info)
> > +	mutex_lock(&ext4_li_mtx);
> > +	if (!ext4_li_info) {
> > +		mutex_unlock(&ext4_li_mtx);
> >  		return;
> > +	}
> >  
> >  	mutex_lock(&ext4_li_info->li_list_mtx);
> > -	ext4_remove_li_request(elr);
> > +	ext4_remove_li_request(EXT4_SB(sb)->s_li_request);
> >  	mutex_unlock(&ext4_li_info->li_list_mtx);
> > +	mutex_unlock(&ext4_li_mtx);
> >  }
> >  
> >  static struct task_struct *ext4_lazyinit_task;
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - May 20, 2011, 4:03 p.m.
Lukas, are you going to be providing a new version of these patch
series or not?  

If you are, could you do it as a separate patch series, instead of
only updating one patch as a reply to the mail thread.  When people do
this, I find it painful since I need to figure out, "ok, I need v2 of
the 1/4 patch, v3 of the 2/4 patch, v4 of the 3/4 patch, and v3 of of
the 4/4 patch.  To provide context, please add version descriptors
after the --- of the patch.  (i.e, "v3 --> v4; fixed commit message")

Also, if we're going to be doing extended review of patches like this,
instead of my just fixing things up when I pull stuff in, people need
to start authoring patches ***much*** sooner.  Doing multiple publish
and review cycles now that the merge window is open really doesn't
work.  One way of solving this in the future is to simply not take any
patch that is first submitted after say, -rc5 or -rc6 until the next
merge window.  But given that some patches didn't *start* getting much
review until 2-3 weeks ago, that wouldn't be entirely fair.

But for the next merge window, if this is going to work, we need
people submitting patches earlier, and people reviewing patches
earlier.

Thanks,

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sandeen - May 20, 2011, 4:12 p.m.
On 5/20/11 11:03 AM, Ted Ts'o wrote:
> Lukas, are you going to be providing a new version of these patch
> series or not?  
> 
> If you are, could you do it as a separate patch series, instead of
> only updating one patch as a reply to the mail thread.  When people do
> this, I find it painful since I need to figure out, "ok, I need v2 of
> the 1/4 patch, v3 of the 2/4 patch, v4 of the 3/4 patch, and v3 of of
> the 4/4 patch.  To provide context, please add version descriptors
> after the --- of the patch.  (i.e, "v3 --> v4; fixed commit message")
> 
> Also, if we're going to be doing extended review of patches like this,
> instead of my just fixing things up when I pull stuff in, people need
> to start authoring patches ***much*** sooner.  Doing multiple publish
> and review cycles now that the merge window is open really doesn't
> work.  One way of solving this in the future is to simply not take any
> patch that is first submitted after say, -rc5 or -rc6 until the next
> merge window.  But given that some patches didn't *start* getting much
> review until 2-3 weeks ago, that wouldn't be entirely fair.
> 
> But for the next merge window, if this is going to work, we need
> people submitting patches earlier, and people reviewing patches
> earlier.

How about a reasonable sounding convention like: if it's non-critical
it's too late, but if it's critical you'll try to get it in.

Windows are windows, reviews are reviews, and if it's too late,
it's too late ... You ultimately get to decide what goes in
and when.

But skipping thorough review simply because the window is open now
doesn't make sense to me.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Czerner - May 20, 2011, 4:16 p.m.
Hi Ted,

On Fri, 20 May 2011, Ted Ts'o wrote:

> Lukas, are you going to be providing a new version of these patch
> series or not?  

I already did send it as a separate patch series with the version
description.

> 
> If you are, could you do it as a separate patch series, instead of
> only updating one patch as a reply to the mail thread.  When people do
> this, I find it painful since I need to figure out, "ok, I need v2 of
> the 1/4 patch, v3 of the 2/4 patch, v4 of the 3/4 patch, and v3 of of
> the 4/4 patch.  To provide context, please add version descriptors
> after the --- of the patch.  (i.e, "v3 --> v4; fixed commit message")
> 
> Also, if we're going to be doing extended review of patches like this,
> instead of my just fixing things up when I pull stuff in, people need
> to start authoring patches ***much*** sooner.  Doing multiple publish
> and review cycles now that the merge window is open really doesn't
> work.  One way of solving this in the future is to simply not take any
> patch that is first submitted after say, -rc5 or -rc6 until the next
> merge window.  But given that some patches didn't *start* getting much
> review until 2-3 weeks ago, that wouldn't be entirely fair.

We all should do better, but I am not sure that limits like that are very
useful. Some level of flexibility is always needed.

> 
> But for the next merge window, if this is going to work, we need
> people submitting patches earlier, and people reviewing patches
> earlier.

So let's to that people ;).

> 
> Thanks,
> 
> 						- Ted

Thanks Ted!
-Lukas

> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Theodore Ts'o - May 20, 2011, 5:39 p.m.
On Fri, May 20, 2011 at 06:16:34PM +0200, Lukas Czerner wrote:
> 
> I already did send it as a separate patch series with the version
> description.

What, you mean the -v2 patch series?

But Eric has made further suggestions where you said you might try to
do more work.  Or did I misread the mail thread?  This is why I hate
it when you reply to the mail thread with an updated patch.  It makes
it very hard to figure out what's going on....

So let me rephrase the question:  Is -v2 your final answer?

       	  	       		     	 - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - May 20, 2011, 5:42 p.m.
On Fri, May 20, 2011 at 01:39:46PM -0400, Ted Ts'o wrote:
> On Fri, May 20, 2011 at 06:16:34PM +0200, Lukas Czerner wrote:
> > 
> > I already did send it as a separate patch series with the version
> > description.
> 
> What, you mean the -v2 patch series?
> 
> 
> So let me rephrase the question:  Is -v2 your final answer?

Ah, never mind.  I got confused when with the other patch series where
half the patches where -v3, and the other half were -v2.  It makes
what shows up in patchwork hard to follow.  In the future, please bump
the version number on the entire series, not just on individual
patches, as it makes it a lot easier for me to track what is going
on....

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - May 20, 2011, 5:47 p.m.
On Fri, May 20, 2011 at 11:12:05AM -0500, Eric Sandeen wrote:
> 
> But skipping thorough review simply because the window is open now
> doesn't make sense to me.

Sure, but adding a 24-hour turnaround for at least simple fix ups
(commit descriptions, comments, white space, etc) doesn't make a lot
of sense either.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sandeen - May 20, 2011, 5:49 p.m.
On 5/20/11 12:47 PM, Ted Ts'o wrote:
> On Fri, May 20, 2011 at 11:12:05AM -0500, Eric Sandeen wrote:
>>
>> But skipping thorough review simply because the window is open now
>> doesn't make sense to me.
> 
> Sure, but adding a 24-hour turnaround for at least simple fix ups
> (commit descriptions, comments, white space, etc) doesn't make a lot
> of sense either.
> 
> 						- Ted

I think you are free to use your discretion for those things :)

I mean if the last review comment is "you have a typo here" and
you really want to make the window, it seems quite reasonable to
fix it up, with a note in the commit log.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c379af6..6a8e48f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2721,14 +2721,16 @@  static void ext4_remove_li_request(struct ext4_li_request *elr)
 
 static void ext4_unregister_li_request(struct super_block *sb)
 {
-	struct ext4_li_request *elr = EXT4_SB(sb)->s_li_request;
-
-	if (!ext4_li_info)
+	mutex_lock(&ext4_li_mtx);
+	if (!ext4_li_info) {
+		mutex_unlock(&ext4_li_mtx);
 		return;
+	}
 
 	mutex_lock(&ext4_li_info->li_list_mtx);
-	ext4_remove_li_request(elr);
+	ext4_remove_li_request(EXT4_SB(sb)->s_li_request);
 	mutex_unlock(&ext4_li_info->li_list_mtx);
+	mutex_unlock(&ext4_li_mtx);
 }
 
 static struct task_struct *ext4_lazyinit_task;