Question/problem with mmu_notifier_unregister.

Andrea,

On a community or upcoming distro based kernel, I have one XPMEM test which is failing. The following patch (with lots of context) fixes it. I do not yet understand the cause of the race condition. I did hope it would be obvious to you and save me the time of investigating further.

The first test finds mn->hlist is on the chain. The new second test does not. I have verified that neither xpmem nor gru is calling to unregister the same mmu_notifier twice. It is very difficult to find the problem as the test case requires a very long time to trip. To increase the likelihood, I run many copies in parallel. Each is 2 processes each with one pthread. I run two per socket on an 8 core per socket system with 256 socket system. When one job hits the null pointer deref, I can have many thousands of lines of debug before the other jobs stop running. Each process has /dev/gru mapped into their address space. They have used xpmem to attach part of the address space of the other process. The tasks are exiting at approximately the same time. The race seems to be with one pthread hitting the final mmput at about the same time as the other task is getting into the filp_close->flush() callout.

I have a couple other things to work on today, and will likely try to chase this failure more tomorrow. With this patch, I have not been able to trigger any other issues in many hours of testing. The test likewise runs for many hours on a RHEL 6.3 based system and a SLES11 SP2 based system so it might have something to do with the change in srcu locking.

mm/mmu_notifier.c +2 -2

index 8a5ac8c..e2c9827 100644
@@ -297,37 +299,38 @@ void mmu_notifier_unregister(struct mmu_notifier *mn, struct mm_struct *mm)
        if (!hlist_unhashed(&mn->hlist)) {
                /*
                 * SRCU here will force exit_mmap to wait ->release to finish
                 * before freeing the pages.
                 */
                int id;
 
                id = srcu_read_lock(&srcu);
                /*
                 * exit_mmap will block in mmu_notifier_release to
                 * guarantee ->release is called before freeing the
                 * pages.
                 */
                if (mn->ops->release)
                        mn->ops->release(mn, mm);
                srcu_read_unlock(&srcu, id);
 
                spin_lock(&mm->mmu_notifier_mm->lock);
-               hlist_del_rcu(&mn->hlist);
+               if (!hlist_unhashed(&mn->hlist))
+                       hlist_del_rcu(&mn->hlist);
                spin_unlock(&mm->mmu_notifier_mm->lock);
        }
 
        /*
         * Wait any running method to finish, of course including
         * ->release if it was run by mmu_notifier_relase instead of us.
         */
        synchronize_srcu(&srcu);
 
        BUG_ON(atomic_read(&mm->mm_count) <= 0);
 
        mmdrop(mm);
 }
 EXPORT_SYMBOL_GPL(mmu_notifier_unregister);
 
 static int __init mmu_notifier_init(void)
 {
        return init_srcu_struct(&srcu);
 
--

To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

There is a race condition between mmu_notifier_unregister() and __mmu_notifier_release().

Assume two tasks, one calling mmu_notifier_unregister() as a result of a filp_close() ->flush() callout (task A), and the other calling mmu_notifier_release() from an mmput() (task B).

A B
t1 srcu_read_lock()
t2 if (!hlist_unhashed())
t3 srcu_read_unlock()
t4 srcu_read_lock()
t5 hlist_del_init_rcu()
t6 synchronize_srcu()
t7 srcu_read_unlock()
t8 hlist_del_rcu() <--- NULL pointer deref.

Tested with this patch applied. My test case which was failing approximately every 300th iteration passed 25,000 tests.

Quoted 8 lines · Robin Holt

There is a race condition between mmu_notifier_unregister() and __mmu_notifier_release().

Assume two tasks, one calling mmu_notifier_unregister() as a result of a filp_close() ->flush() callout (task A), and the other calling mmu_notifier_release() from an mmput() (task B).

Quoted 9 lines

A B
t1 srcu_read_lock()
t2 if (!hlist_unhashed())
t3 srcu_read_unlock()
t4 srcu_read_lock()
t5 hlist_del_init_rcu()
t6 synchronize_srcu()
t7 srcu_read_unlock()
t8 hlist_del_rcu() <--- NULL pointer deref.

The detailed code here is:

	hlist_del_rcu(&mn->hlist);

Can mn be NULL? I do not think so since mn is always the embedded struct of the caller, it be freed after calling mmu_notifier_unregister.

Quoted 4 lines

Tested with this patch applied. My test case which was failing approximately every 300th iteration passed 25,000 tests.

Could you please share your test case?

Quoted 22 lines · Xiao Guangrong

Quoted 8 lines · Robin Holt

There is a race condition between mmu_notifier_unregister() and __mmu_notifier_release().

Assume two tasks, one calling mmu_notifier_unregister() as a result of a filp_close() ->flush() callout (task A), and the other calling mmu_notifier_release() from an mmput() (task B).

Quoted 9 lines

A B
t1 srcu_read_lock()
t2 if (!hlist_unhashed())
t3 srcu_read_unlock()
t4 srcu_read_lock()
t5 hlist_del_init_rcu()
t6 synchronize_srcu()
t7 srcu_read_unlock()
t8 hlist_del_rcu() <--- NULL pointer deref.

The detailed code here is:

	hlist_del_rcu(&mn->hlist);

Quoted 2 lines

Can mn be NULL? I do not think so since mn is always the embedded struct of the caller, it be freed after calling mmu_notifier_unregister.

If you look at __mmu_notifier_release() it is using hlist_del_init_rcu() which will set the hlist->pprev to NULL. When hlist_del_rcu() is called, it attempts to update *hlist->pprev = hlist->next and that is where it takes the NULL pointer deref.

Quoted 6 lines

Quoted 5 lines

Tested with this patch applied. My test case which was failing approximately every 300th iteration passed 25,000 tests.

Quoted 1 line

Could you please share your test case?

I could but it would be very useless. It depends upon having a SGI UV system with GRUs and and xpmem kernel module loaded. If you would really like all the bits, I could provide them, but you will not be able to reproduce the failure.

Quoted 26 lines · Robin Holt

Quoted 24 lines · Xiao Guangrong

Quoted 8 lines · Robin Holt

There is a race condition between mmu_notifier_unregister() and __mmu_notifier_release().

Assume two tasks, one calling mmu_notifier_unregister() as a result of a filp_close() ->flush() callout (task A), and the other calling mmu_notifier_release() from an mmput() (task B).

Quoted 9 lines

A B
t1 srcu_read_lock()
t2 if (!hlist_unhashed())
t3 srcu_read_unlock()
t4 srcu_read_lock()
t5 hlist_del_init_rcu()
t6 synchronize_srcu()
t7 srcu_read_unlock()
t8 hlist_del_rcu() <--- NULL pointer deref.

The detailed code here is:

	hlist_del_rcu(&mn->hlist);

Can mn be NULL? I do not think so since mn is always the embedded struct of the caller, it be freed after calling mmu_notifier_unregister.

Quoted 4 lines

If you look at __mmu_notifier_release() it is using hlist_del_init_rcu() which will set the hlist->pprev to NULL. When hlist_del_rcu() is called, it attempts to update *hlist->pprev = hlist->next and that is where it takes the NULL pointer deref.

Yes, sorry for my careless. So, That can not be fixed by using hlist_del_init_rcu instead?

Quoted 27 lines · Xiao Guangrong

Quoted 26 lines · Robin Holt

Quoted 24 lines · Xiao Guangrong

Quoted 8 lines · Robin Holt

There is a race condition between mmu_notifier_unregister() and __mmu_notifier_release().

Assume two tasks, one calling mmu_notifier_unregister() as a result of a filp_close() ->flush() callout (task A), and the other calling mmu_notifier_release() from an mmput() (task B).

Quoted 9 lines

A B
t1 srcu_read_lock()
t2 if (!hlist_unhashed())
t3 srcu_read_unlock()
t4 srcu_read_lock()
t5 hlist_del_init_rcu()
t6 synchronize_srcu()
t7 srcu_read_unlock()
t8 hlist_del_rcu() <--- NULL pointer deref.

The detailed code here is:

	hlist_del_rcu(&mn->hlist);

Can mn be NULL? I do not think so since mn is always the embedded struct of the caller, it be freed after calling mmu_notifier_unregister.

Quoted 7 lines

Quoted 4 lines

If you look at __mmu_notifier_release() it is using hlist_del_init_rcu() which will set the hlist->pprev to NULL. When hlist_del_rcu() is called, it attempts to update *hlist->pprev = hlist->next and that is where it takes the NULL pointer deref.

Yes, sorry for my careless. So, That can not be fixed by using hlist_del_init_rcu instead?

The problem is the race described above. Thread 'A' has checked to see if n->pprev != NULL. Based upon that, it did called the mn->release() method. While it was trying to call the release method, thread 'B' ended up calling hlist_del_init_rcu() which set n->pprev = NULL. Then thread 'A' got to run again and now it tries to do the hlist_del_rcu() which, as part of __hlist_del(), the pprev will be set to n->pprev (which is NULL) and then *pprev = n->next; hits the NULL pointer deref hits.

Quoted 36 lines · Robin Holt

Quoted 27 lines · Xiao Guangrong

Quoted 26 lines · Robin Holt

Quoted 24 lines · Xiao Guangrong

Quoted 8 lines · Robin Holt

There is a race condition between mmu_notifier_unregister() and __mmu_notifier_release().

Assume two tasks, one calling mmu_notifier_unregister() as a result of a filp_close() ->flush() callout (task A), and the other calling mmu_notifier_release() from an mmput() (task B).

Quoted 9 lines

A B
t1 srcu_read_lock()
t2 if (!hlist_unhashed())
t3 srcu_read_unlock()
t4 srcu_read_lock()
t5 hlist_del_init_rcu()
t6 synchronize_srcu()
t7 srcu_read_unlock()
t8 hlist_del_rcu() <--- NULL pointer deref.

The detailed code here is:

	hlist_del_rcu(&mn->hlist);

Can mn be NULL? I do not think so since mn is always the embedded struct of the caller, it be freed after calling mmu_notifier_unregister.

Quoted 7 lines

Quoted 4 lines

If you look at __mmu_notifier_release() it is using hlist_del_init_rcu() which will set the hlist->pprev to NULL. When hlist_del_rcu() is called, it attempts to update *hlist->pprev = hlist->next and that is where it takes the NULL pointer deref.

Yes, sorry for my careless. So, That can not be fixed by using hlist_del_init_rcu instead?

Quoted 7 lines

The problem is the race described above. Thread 'A' has checked to see if n->pprev != NULL. Based upon that, it did called the mn->release() method. While it was trying to call the release method, thread 'B' ended up calling hlist_del_init_rcu() which set n->pprev = NULL. Then thread 'A' got to run again and now it tries to do the hlist_del_rcu() which, as part of __hlist_del(), the pprev will be set to n->pprev (which is NULL) and then *pprev = n->next; hits the NULL pointer deref hits.

I mean using hlist_del_init_rcu instead of hlist_del_rcu in mmu_notifier_unregister(), hlist_del_init_rcu is aware of ->pprev.

Quoted 45 lines · Xiao Guangrong

Quoted 43 lines · Robin Holt

Quoted 27 lines · Xiao Guangrong

Quoted 26 lines · Robin Holt

Quoted 24 lines · Xiao Guangrong

Quoted 8 lines · Robin Holt

There is a race condition between mmu_notifier_unregister() and __mmu_notifier_release().

Assume two tasks, one calling mmu_notifier_unregister() as a result of a filp_close() ->flush() callout (task A), and the other calling mmu_notifier_release() from an mmput() (task B).

Quoted 9 lines

A B
t1 srcu_read_lock()
t2 if (!hlist_unhashed())
t3 srcu_read_unlock()
t4 srcu_read_lock()
t5 hlist_del_init_rcu()
t6 synchronize_srcu()
t7 srcu_read_unlock()
t8 hlist_del_rcu() <--- NULL pointer deref.

The detailed code here is:

	hlist_del_rcu(&mn->hlist);

Can mn be NULL? I do not think so since mn is always the embedded struct of the caller, it be freed after calling mmu_notifier_unregister.

Quoted 7 lines

Quoted 4 lines

If you look at __mmu_notifier_release() it is using hlist_del_init_rcu() which will set the hlist->pprev to NULL. When hlist_del_rcu() is called, it attempts to update *hlist->pprev = hlist->next and that is where it takes the NULL pointer deref.

Yes, sorry for my careless. So, That can not be fixed by using hlist_del_init_rcu instead?

The problem is the race described above. Thread 'A' has checked to see if n->pprev != NULL. Based upon that, it did called the mn->release() method. While it was trying to call the release method, thread 'B' ended up calling hlist_del_init_rcu() which set n->pprev = NULL. Then thread 'A' got to run again and now it tries to do the hlist_del_rcu() which, as part of __hlist_del(), the pprev will be set to n->pprev (which is NULL) and then *pprev = n->next; hits the NULL pointer deref hits.

Quoted 2 lines

I mean using hlist_del_init_rcu instead of hlist_del_rcu in mmu_notifier_unregister(), hlist_del_init_rcu is aware of ->pprev.

How does that address the calling of the ->release() method twice?

Quoted 46 lines · Robin Holt

Quoted 45 lines · Xiao Guangrong

Quoted 43 lines · Robin Holt

Quoted 27 lines · Xiao Guangrong

Quoted 26 lines · Robin Holt

Quoted 24 lines · Xiao Guangrong

Quoted 8 lines · Robin Holt

There is a race condition between mmu_notifier_unregister() and __mmu_notifier_release().

Assume two tasks, one calling mmu_notifier_unregister() as a result of a filp_close() ->flush() callout (task A), and the other calling mmu_notifier_release() from an mmput() (task B).

Quoted 9 lines

A B
t1 srcu_read_lock()
t2 if (!hlist_unhashed())
t3 srcu_read_unlock()
t4 srcu_read_lock()
t5 hlist_del_init_rcu()
t6 synchronize_srcu()
t7 srcu_read_unlock()
t8 hlist_del_rcu() <--- NULL pointer deref.

The detailed code here is:

	hlist_del_rcu(&mn->hlist);

Can mn be NULL? I do not think so since mn is always the embedded struct of the caller, it be freed after calling mmu_notifier_unregister.

Quoted 7 lines

Quoted 4 lines

If you look at __mmu_notifier_release() it is using hlist_del_init_rcu() which will set the hlist->pprev to NULL. When hlist_del_rcu() is called, it attempts to update *hlist->pprev = hlist->next and that is where it takes the NULL pointer deref.

Yes, sorry for my careless. So, That can not be fixed by using hlist_del_init_rcu instead?

The problem is the race described above. Thread 'A' has checked to see if n->pprev != NULL. Based upon that, it did called the mn->release() method. While it was trying to call the release method, thread 'B' ended up calling hlist_del_init_rcu() which set n->pprev = NULL. Then thread 'A' got to run again and now it tries to do the hlist_del_rcu() which, as part of __hlist_del(), the pprev will be set to n->pprev (which is NULL) and then *pprev = n->next; hits the NULL pointer deref hits.

Quoted 4 lines

Quoted 2 lines

I mean using hlist_del_init_rcu instead of hlist_del_rcu in mmu_notifier_unregister(), hlist_del_init_rcu is aware of ->pprev.

How does that address the calling of the ->release() method twice?

Hmm, what is the problem of it? If it is just for "performance issue", i think it is not worth introducing so complex lock rule just for the really rare case.

Quoted 52 lines · Xiao Guangrong

Quoted 50 lines · Robin Holt

Quoted 45 lines · Xiao Guangrong

Quoted 36 lines · Robin Holt

Quoted 27 lines · Xiao Guangrong

Quoted 26 lines · Robin Holt

Quoted 24 lines · Xiao Guangrong

Quoted 8 lines · Robin Holt

There is a race condition between mmu_notifier_unregister() and __mmu_notifier_release().

Assume two tasks, one calling mmu_notifier_unregister() as a result of a filp_close() ->flush() callout (task A), and the other calling mmu_notifier_release() from an mmput() (task B).

Quoted 9 lines

A B
t1 srcu_read_lock()
t2 if (!hlist_unhashed())
t3 srcu_read_unlock()
t4 srcu_read_lock()
t5 hlist_del_init_rcu()
t6 synchronize_srcu()
t7 srcu_read_unlock()
t8 hlist_del_rcu() <--- NULL pointer deref.

The detailed code here is:

	hlist_del_rcu(&mn->hlist);

Can mn be NULL? I do not think so since mn is always the embedded struct of the caller, it be freed after calling mmu_notifier_unregister.

Quoted 7 lines

Quoted 4 lines

If you look at __mmu_notifier_release() it is using hlist_del_init_rcu() which will set the hlist->pprev to NULL. When hlist_del_rcu() is called, it attempts to update *hlist->pprev = hlist->next and that is where it takes the NULL pointer deref.

Yes, sorry for my careless. So, That can not be fixed by using hlist_del_init_rcu instead?

Quoted 7 lines

The problem is the race described above. Thread 'A' has checked to see if n->pprev != NULL. Based upon that, it did called the mn->release() method. While it was trying to call the release method, thread 'B' ended up calling hlist_del_init_rcu() which set n->pprev = NULL. Then thread 'A' got to run again and now it tries to do the hlist_del_rcu() which, as part of __hlist_del(), the pprev will be set to n->pprev (which is NULL) and then *pprev = n->next; hits the NULL pointer deref hits.

Quoted 2 lines

I mean using hlist_del_init_rcu instead of hlist_del_rcu in mmu_notifier_unregister(), hlist_del_init_rcu is aware of ->pprev.

How does that address the calling of the ->release() method twice?

Quoted 2 lines

Hmm, what is the problem of it? If it is just for "performance issue", i think it is not worth introducing so complex lock rule just for the really rare case.

Complex lock rule? We merely moved the lock up earlier in code path. Without this, we have some cases where you get called on ->release() twice, while the majority of cases your notifier gets called once and it hits a NULL pointer deref at that. What is so complex about that?

Quoted 53 lines · Robin Holt

Quoted 52 lines · Xiao Guangrong

Quoted 50 lines · Robin Holt

Quoted 45 lines · Xiao Guangrong

Quoted 36 lines · Robin Holt

Quoted 27 lines · Xiao Guangrong

Quoted 26 lines · Robin Holt

Quoted 24 lines · Xiao Guangrong

Quoted 8 lines · Robin Holt

There is a race condition between mmu_notifier_unregister() and __mmu_notifier_release().

Assume two tasks, one calling mmu_notifier_unregister() as a result of a filp_close() ->flush() callout (task A), and the other calling mmu_notifier_release() from an mmput() (task B).

Quoted 9 lines

A B
t1 srcu_read_lock()
t2 if (!hlist_unhashed())
t3 srcu_read_unlock()
t4 srcu_read_lock()
t5 hlist_del_init_rcu()
t6 synchronize_srcu()
t7 srcu_read_unlock()
t8 hlist_del_rcu() <--- NULL pointer deref.

The detailed code here is:

	hlist_del_rcu(&mn->hlist);

Can mn be NULL? I do not think so since mn is always the embedded struct of the caller, it be freed after calling mmu_notifier_unregister.

Quoted 7 lines

Quoted 4 lines

If you look at __mmu_notifier_release() it is using hlist_del_init_rcu() which will set the hlist->pprev to NULL. When hlist_del_rcu() is called, it attempts to update *hlist->pprev = hlist->next and that is where it takes the NULL pointer deref.

Yes, sorry for my careless. So, That can not be fixed by using hlist_del_init_rcu instead?

Quoted 7 lines

The problem is the race described above. Thread 'A' has checked to see if n->pprev != NULL. Based upon that, it did called the mn->release() method. While it was trying to call the release method, thread 'B' ended up calling hlist_del_init_rcu() which set n->pprev = NULL. Then thread 'A' got to run again and now it tries to do the hlist_del_rcu() which, as part of __hlist_del(), the pprev will be set to n->pprev (which is NULL) and then *pprev = n->next; hits the NULL pointer deref hits.

Quoted 2 lines

I mean using hlist_del_init_rcu instead of hlist_del_rcu in mmu_notifier_unregister(), hlist_del_init_rcu is aware of ->pprev.

How does that address the calling of the ->release() method twice?

Quoted 7 lines

Quoted 2 lines

Hmm, what is the problem of it? If it is just for "performance issue", i think it is not worth introducing so complex lock rule just for the really rare case.

Complex lock rule? We merely moved the lock up earlier in code path. Without this, we have some cases where you get called on ->release() twice, while the majority of cases your notifier gets called once and it hits a NULL pointer deref at that. What is so complex about that?

Aha, if we use hlist_del_init_rcu() instead of hlist_del_rcu, can the NULL deref bug be fixed?

- If yes, you'd better make it as a simple patch, it is good for backport. Then make the second patch to fix the "problem" of calling ->release twice.

- if no. Could you please detail the changelog. From the changelog, i only see the bug is cased by calling hlist_del_rcu on the unhashed node.

Quoted 63 lines · Xiao Guangrong

Quoted 53 lines · Robin Holt

Quoted 52 lines · Xiao Guangrong

Quoted 50 lines · Robin Holt

Quoted 45 lines · Xiao Guangrong

Quoted 36 lines · Robin Holt

Quoted 27 lines · Xiao Guangrong

Quoted 26 lines · Robin Holt

Quoted 24 lines · Xiao Guangrong

Quoted 8 lines · Robin Holt

There is a race condition between mmu_notifier_unregister() and __mmu_notifier_release().

Assume two tasks, one calling mmu_notifier_unregister() as a result of a filp_close() ->flush() callout (task A), and the other calling mmu_notifier_release() from an mmput() (task B).

Quoted 9 lines

A B
t1 srcu_read_lock()
t2 if (!hlist_unhashed())
t3 srcu_read_unlock()
t4 srcu_read_lock()
t5 hlist_del_init_rcu()
t6 synchronize_srcu()
t7 srcu_read_unlock()
t8 hlist_del_rcu() <--- NULL pointer deref.

The detailed code here is:

	hlist_del_rcu(&mn->hlist);

Can mn be NULL? I do not think so since mn is always the embedded struct of the caller, it be freed after calling mmu_notifier_unregister.

Quoted 7 lines

Quoted 4 lines

If you look at __mmu_notifier_release() it is using hlist_del_init_rcu() which will set the hlist->pprev to NULL. When hlist_del_rcu() is called, it attempts to update *hlist->pprev = hlist->next and that is where it takes the NULL pointer deref.

Yes, sorry for my careless. So, That can not be fixed by using hlist_del_init_rcu instead?

Quoted 7 lines

The problem is the race described above. Thread 'A' has checked to see if n->pprev != NULL. Based upon that, it did called the mn->release() method. While it was trying to call the release method, thread 'B' ended up calling hlist_del_init_rcu() which set n->pprev = NULL. Then thread 'A' got to run again and now it tries to do the hlist_del_rcu() which, as part of __hlist_del(), the pprev will be set to n->pprev (which is NULL) and then *pprev = n->next; hits the NULL pointer deref hits.

Quoted 2 lines

I mean using hlist_del_init_rcu instead of hlist_del_rcu in mmu_notifier_unregister(), hlist_del_init_rcu is aware of ->pprev.

How does that address the calling of the ->release() method twice?

Quoted 7 lines

Quoted 2 lines

Hmm, what is the problem of it? If it is just for "performance issue", i think it is not worth introducing so complex lock rule just for the really rare case.

Complex lock rule? We merely moved the lock up earlier in code path. Without this, we have some cases where you get called on ->release() twice, while the majority of cases your notifier gets called once and it hits a NULL pointer deref at that. What is so complex about that?

Quoted 8 lines

Aha, if we use hlist_del_init_rcu() instead of hlist_del_rcu, can the NULL deref bug be fixed?

- If yes, you'd better make it as a simple patch, it is good for backport. Then make the second patch to fix the "problem" of calling ->release twice.

- if no. Could you please detail the changelog. From the changelog, i only see the bug is cased by calling hlist_del_rcu on the unhashed node.

What is it about this patch makes you think it is complex? There are:

1) 11 Lines relocated.
2) 5 new lines added (4 four an optimization, 1 for "else" case).
3) 5 blank line introduced.
4) 1 comment line fixed.

I would happily remove the optimization which brings us down to an else case. That could be removed by introducing a temporary variable as well, but that seems pointless.

Bottom line, I do not see how this patch, as-is or with some slight tweaking, is not already candidate material for the -stable trees. It is certainly not complex and significantly improves an inconsistency with how the unregister notifier has worked for a few years. It is a new behavior which is contrary to the comments in mmu_notifier.h which says:

* Called either by mmu_notifier_unregister or when the mm is
* being destroyed by exit_mmap, always before all pages are
...
void (*release)(struct mmu_notifier *mn,

That does not say "and/or", it says "or" which used to be "once and only once", but is now "once, unless it is twice". This new behavior was affecting some of our test jobs, but the failures were so sporadic that we were not making any progress on identifying the failures until we stumbled on a test case which more frequently failed, then refined that test to trigger easily.

My reluctance to not improving the double callout is I would need to repeat a significant amount of testing to ensure the other problems are also fixed with just the removal of the NULL pointer deref. I believe they are, but I am not certain.

Thanks,
Robin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 63 lines · Xiao Guangrong

Quoted 53 lines · Robin Holt

Quoted 52 lines · Xiao Guangrong

Quoted 50 lines · Robin Holt

Quoted 45 lines · Xiao Guangrong

Quoted 36 lines · Robin Holt

Quoted 27 lines · Xiao Guangrong

Quoted 26 lines · Robin Holt

Quoted 24 lines · Xiao Guangrong

Quoted 8 lines · Robin Holt

There is a race condition between mmu_notifier_unregister() and __mmu_notifier_release().

Assume two tasks, one calling mmu_notifier_unregister() as a result of a filp_close() ->flush() callout (task A), and the other calling mmu_notifier_release() from an mmput() (task B).

Quoted 9 lines

A B
t1 srcu_read_lock()
t2 if (!hlist_unhashed())
t3 srcu_read_unlock()
t4 srcu_read_lock()
t5 hlist_del_init_rcu()
t6 synchronize_srcu()
t7 srcu_read_unlock()
t8 hlist_del_rcu() <--- NULL pointer deref.

The detailed code here is:

	hlist_del_rcu(&mn->hlist);

Can mn be NULL? I do not think so since mn is always the embedded struct of the caller, it be freed after calling mmu_notifier_unregister.

Quoted 7 lines

Quoted 4 lines

If you look at __mmu_notifier_release() it is using hlist_del_init_rcu() which will set the hlist->pprev to NULL. When hlist_del_rcu() is called, it attempts to update *hlist->pprev = hlist->next and that is where it takes the NULL pointer deref.

Yes, sorry for my careless. So, That can not be fixed by using hlist_del_init_rcu instead?

Quoted 7 lines

The problem is the race described above. Thread 'A' has checked to see if n->pprev != NULL. Based upon that, it did called the mn->release() method. While it was trying to call the release method, thread 'B' ended up calling hlist_del_init_rcu() which set n->pprev = NULL. Then thread 'A' got to run again and now it tries to do the hlist_del_rcu() which, as part of __hlist_del(), the pprev will be set to n->pprev (which is NULL) and then *pprev = n->next; hits the NULL pointer deref hits.

Quoted 2 lines

I mean using hlist_del_init_rcu instead of hlist_del_rcu in mmu_notifier_unregister(), hlist_del_init_rcu is aware of ->pprev.

How does that address the calling of the ->release() method twice?

Quoted 7 lines

Quoted 2 lines

Hmm, what is the problem of it? If it is just for "performance issue", i think it is not worth introducing so complex lock rule just for the really rare case.

Complex lock rule? We merely moved the lock up earlier in code path. Without this, we have some cases where you get called on ->release() twice, while the majority of cases your notifier gets called once and it hits a NULL pointer deref at that. What is so complex about that?

Quoted 8 lines

Aha, if we use hlist_del_init_rcu() instead of hlist_del_rcu, can the NULL deref bug be fixed?

- If yes, you'd better make it as a simple patch, it is good for backport. Then make the second patch to fix the "problem" of calling ->release twice.

- if no. Could you please detail the changelog. From the changelog, i only see the bug is cased by calling hlist_del_rcu on the unhashed node.

What is it about this patch makes you think it is complex? There are:

1) 11 Lines relocated.
2) 5 new lines added (4 four an optimization, 1 for "else" case).
3) 5 blank line introduced.
4) 1 comment line fixed.

I would happily remove the optimization which brings us down to an else case. That could be removed by introducing a temporary variable as well, but that seems pointless.

Bottom line, I do not see how this patch, as-is or with some slight tweaking, is not already candidate material for the -stable trees. It is certainly not complex and significantly improves an inconsistency with how the unregister notifier has worked for a few years. It is a new behavior which is contrary to the comments in mmu_notifier.h which says:

* Called either by mmu_notifier_unregister or when the mm is
* being destroyed by exit_mmap, always before all pages are
...
void (*release)(struct mmu_notifier *mn,

That does not say "and/or", it says "or" which used to be "once and only once", but is now "once, unless it is twice". This new behavior was affecting some of our test jobs, but the failures were so sporadic that we were not making any progress on identifying the failures until we stumbled on a test case which more frequently failed, then refined that test to trigger easily.

My reluctance to not improving the double callout is I would need to repeat a significant amount of testing to ensure the other problems are also fixed with just the removal of the NULL pointer deref. I believe they are, but I am not certain.

Thanks,
Robin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>