Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On 2019/7/17 19:15, Vasily Averin wrote: > On 7/16/19 5:07 PM, Xiaoming Ni wrote: >> On 2019/7/16 18:20, Vasily Averin wrote: >>> On 7/16/19 5:00 AM, Xiaoming Ni wrote: On 2019/7/15 13:38, Vasily Averin wrote: > On 7/14/19 5:45 AM, Xiaoming Ni wrote: >> On 2019/7/12 22:07, gre...@linuxfoundation.org wrote: >>> On Fri, Jul 12, 2019 at 09:11:57PM +0800, Xiaoming Ni wrote: On 2019/7/11 21:57, Vasily Averin wrote: > On 7/11/19 4:55 AM, Nixiaoming wrote: >> On Wed, July 10, 2019 1:49 PM Vasily Averin wrote: >>> On 7/10/19 6:09 AM, Xiaoming Ni wrote: ... So in these two cases, is it more reasonable to trigger BUG() directly when checking for duplicate registration ? But why does current notifier_chain_register() just trigger WARN() without exiting ? notifier_chain_cond_register() direct exit without triggering WARN() ? >>> >>> It should recover from this, if it can be detected. The main point is >>> that not all apis have to be this "robust" when used within the kernel >>> as we do allow for the callers to know what they are doing :) >>> >> In the notifier_chain_register(), the condition ( (*nl) == n) is the >> same registration of the same hook. >> We can intercept this situation and avoid forming a linked list ring to >> make the API more rob ... ... > Yes, I'm agree, at present there are no difference between > notifier_chain_cond_register() and notifier_chain_register() > > Question is -- how to improve it. > You propose to remove notifier_chain_cond_register() by some way. > Another option is return an error, for some abstract callers who expect > possible double registration. > > Frankly speaking I prefer second one, > however because of kernel do not have any such callers right now seems you > are right, > and we can delete notifier_chain_cond_register(). > > So let me finally accept your patch-set. > > Thank you, > Vasily Averin > > . > Dear Greg Kroah-Hartman is there any other opinion on this patch set? can you pick this series? thanks Xiaoming Ni
Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On 7/16/19 5:07 PM, Xiaoming Ni wrote:
> On 2019/7/16 18:20, Vasily Averin wrote:
>> On 7/16/19 5:00 AM, Xiaoming Ni wrote:
>>> On 2019/7/15 13:38, Vasily Averin wrote:
On 7/14/19 5:45 AM, Xiaoming Ni wrote:
> On 2019/7/12 22:07, gre...@linuxfoundation.org wrote:
>> On Fri, Jul 12, 2019 at 09:11:57PM +0800, Xiaoming Ni wrote:
>>> On 2019/7/11 21:57, Vasily Averin wrote:
On 7/11/19 4:55 AM, Nixiaoming wrote:
> On Wed, July 10, 2019 1:49 PM Vasily Averin wrote:
>> On 7/10/19 6:09 AM, Xiaoming Ni wrote:
>>> Registering the same notifier to a hook repeatedly can cause the
>>> hook
>>> list to form a ring or lose other members of the list.
>>
>> I think is not enough to _prevent_ 2nd register attempt,
>> it's enough to detect just attempt and generate warning to mark host
>> in bad state.
>>
>
> Duplicate registration is prevented in my patch, not just "mark host
> in bad state"
>
> Duplicate registration is checked and exited in
> notifier_chain_cond_register()
>
> Duplicate registration was checked in notifier_chain_register() but
> only
> the alarm was triggered without exiting. added by commit
> 831246570d34692e
> ("kernel/notifier.c: double register detection")
>
> My patch is like a combination of 831246570d34692e and
> notifier_chain_cond_register(),
> which triggers an alarm and exits when a duplicate registration is
> detected.
>
>> Unexpected 2nd register of the same hook most likely will lead to
>> 2nd unregister,
>> and it can lead to host crash in any time:
>> you can unregister notifier on first attempt it can be too early, it
>> can be still in use.
>> on the other hand you can never call 2nd unregister at all.
>
> Since the member was not added to the linked list at the time of the
> second registration,
> no linked list ring was formed.
> The member is released on the first unregistration and -ENOENT on the
> second unregistration.
> After patching, the fault has been alleviated
You are wrong here.
2nd notifier's registration is a pure bug, this should never happen.
If you know the way to reproduce this situation -- you need to fix it.
2nd registration can happen in 2 cases:
1) missed rollback, when someone forget to call unregister after
successfull registration,
and then tried to call register again. It can lead to crash for
example when according module will be unloaded.
2) some subsystem is registered twice, for example from different
namespaces.
in this case unregister called during sybsystem cleanup in first
namespace will incorrectly remove notifier used
in second namespace, it also can lead to unexpacted behaviour.
>>> So in these two cases, is it more reasonable to trigger BUG() directly
>>> when checking for duplicate registration ?
>>> But why does current notifier_chain_register() just trigger WARN()
>>> without exiting ?
>>> notifier_chain_cond_register() direct exit without triggering WARN() ?
>>
>> It should recover from this, if it can be detected. The main point is
>> that not all apis have to be this "robust" when used within the kernel
>> as we do allow for the callers to know what they are doing :)
>>
> In the notifier_chain_register(), the condition ( (*nl) == n) is the same
> registration of the same hook.
> We can intercept this situation and avoid forming a linked list ring to
> make the API more rob
Once again -- yes, you CAN prevent list corruption, but you CANNOT recover
the host and return it back to safe state.
If double register event was detected -- it means you have bug in kernel.
Yes, you can add BUG here and crash the host immediately, but I prefer to
use warning in such situations.
>> If this does not cause any additional problems or slow downs, it's
>> probably fine to add.
>>
> Notifier_chain_register() is not a system hotspot function.
> At the same time, there is already a WARN_ONCE judgment. There is no new
> judgment in the new patch.
> It only changes the processing under the condition of (*nl) == n, which
> will not cause performance problems.
> At the same time, avoiding the formation of a link ring can make the
> system more robust.
I disagree,
yes, node will have correct list, but anyway node will work wrong and can
crash the host in any time.
>>>
>>> Sorry, my description is not accurate.
>>>
>>> My patch feature does not prevent users from repeatedly registering
Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On 2019/7/16 18:20, Vasily Averin wrote:
> On 7/16/19 5:00 AM, Xiaoming Ni wrote:
>> On 2019/7/15 13:38, Vasily Averin wrote:
>>> On 7/14/19 5:45 AM, Xiaoming Ni wrote:
On 2019/7/12 22:07, gre...@linuxfoundation.org wrote:
> On Fri, Jul 12, 2019 at 09:11:57PM +0800, Xiaoming Ni wrote:
>> On 2019/7/11 21:57, Vasily Averin wrote:
>>> On 7/11/19 4:55 AM, Nixiaoming wrote:
On Wed, July 10, 2019 1:49 PM Vasily Averin wrote:
> On 7/10/19 6:09 AM, Xiaoming Ni wrote:
>> Registering the same notifier to a hook repeatedly can cause the hook
>> list to form a ring or lose other members of the list.
>
> I think is not enough to _prevent_ 2nd register attempt,
> it's enough to detect just attempt and generate warning to mark host
> in bad state.
>
Duplicate registration is prevented in my patch, not just "mark host
in bad state"
Duplicate registration is checked and exited in
notifier_chain_cond_register()
Duplicate registration was checked in notifier_chain_register() but
only
the alarm was triggered without exiting. added by commit
831246570d34692e
("kernel/notifier.c: double register detection")
My patch is like a combination of 831246570d34692e and
notifier_chain_cond_register(),
which triggers an alarm and exits when a duplicate registration is
detected.
> Unexpected 2nd register of the same hook most likely will lead to 2nd
> unregister,
> and it can lead to host crash in any time:
> you can unregister notifier on first attempt it can be too early, it
> can be still in use.
> on the other hand you can never call 2nd unregister at all.
Since the member was not added to the linked list at the time of the
second registration,
no linked list ring was formed.
The member is released on the first unregistration and -ENOENT on the
second unregistration.
After patching, the fault has been alleviated
>>>
>>> You are wrong here.
>>> 2nd notifier's registration is a pure bug, this should never happen.
>>> If you know the way to reproduce this situation -- you need to fix it.
>>>
>>> 2nd registration can happen in 2 cases:
>>> 1) missed rollback, when someone forget to call unregister after
>>> successfull registration,
>>> and then tried to call register again. It can lead to crash for example
>>> when according module will be unloaded.
>>> 2) some subsystem is registered twice, for example from different
>>> namespaces.
>>> in this case unregister called during sybsystem cleanup in first
>>> namespace will incorrectly remove notifier used
>>> in second namespace, it also can lead to unexpacted behaviour.
>>>
>> So in these two cases, is it more reasonable to trigger BUG() directly
>> when checking for duplicate registration ?
>> But why does current notifier_chain_register() just trigger WARN()
>> without exiting ?
>> notifier_chain_cond_register() direct exit without triggering WARN() ?
>
> It should recover from this, if it can be detected. The main point is
> that not all apis have to be this "robust" when used within the kernel
> as we do allow for the callers to know what they are doing :)
>
In the notifier_chain_register(), the condition ( (*nl) == n) is the same
registration of the same hook.
We can intercept this situation and avoid forming a linked list ring to
make the API more rob
>>>
>>> Once again -- yes, you CAN prevent list corruption, but you CANNOT recover
>>> the host and return it back to safe state.
>>> If double register event was detected -- it means you have bug in kernel.
>>>
>>> Yes, you can add BUG here and crash the host immediately, but I prefer to
>>> use warning in such situations.
>>>
> If this does not cause any additional problems or slow downs, it's
> probably fine to add.
>
Notifier_chain_register() is not a system hotspot function.
At the same time, there is already a WARN_ONCE judgment. There is no new
judgment in the new patch.
It only changes the processing under the condition of (*nl) == n, which
will not cause performance problems.
At the same time, avoiding the formation of a link ring can make the
system more robust.
>>>
>>> I disagree,
>>> yes, node will have correct list, but anyway node will work wrong and can
>>> crash the host in any time.
>>
>> Sorry, my description is not accurate.
>>
>> My patch feature does not prevent users from repeatedly registering hooks.
>> But avoiding the chain ring caused by the user repeatedly registering the
>> hook
>>
>> There are no modules for duplicate registration hooks in
Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On 7/16/19 5:00 AM, Xiaoming Ni wrote:
> On 2019/7/15 13:38, Vasily Averin wrote:
>> On 7/14/19 5:45 AM, Xiaoming Ni wrote:
>>> On 2019/7/12 22:07, gre...@linuxfoundation.org wrote:
On Fri, Jul 12, 2019 at 09:11:57PM +0800, Xiaoming Ni wrote:
> On 2019/7/11 21:57, Vasily Averin wrote:
>> On 7/11/19 4:55 AM, Nixiaoming wrote:
>>> On Wed, July 10, 2019 1:49 PM Vasily Averin wrote:
On 7/10/19 6:09 AM, Xiaoming Ni wrote:
> Registering the same notifier to a hook repeatedly can cause the hook
> list to form a ring or lose other members of the list.
I think is not enough to _prevent_ 2nd register attempt,
it's enough to detect just attempt and generate warning to mark host
in bad state.
>>>
>>> Duplicate registration is prevented in my patch, not just "mark host in
>>> bad state"
>>>
>>> Duplicate registration is checked and exited in
>>> notifier_chain_cond_register()
>>>
>>> Duplicate registration was checked in notifier_chain_register() but
>>> only
>>> the alarm was triggered without exiting. added by commit
>>> 831246570d34692e
>>> ("kernel/notifier.c: double register detection")
>>>
>>> My patch is like a combination of 831246570d34692e and
>>> notifier_chain_cond_register(),
>>> which triggers an alarm and exits when a duplicate registration is
>>> detected.
>>>
Unexpected 2nd register of the same hook most likely will lead to 2nd
unregister,
and it can lead to host crash in any time:
you can unregister notifier on first attempt it can be too early, it
can be still in use.
on the other hand you can never call 2nd unregister at all.
>>>
>>> Since the member was not added to the linked list at the time of the
>>> second registration,
>>> no linked list ring was formed.
>>> The member is released on the first unregistration and -ENOENT on the
>>> second unregistration.
>>> After patching, the fault has been alleviated
>>
>> You are wrong here.
>> 2nd notifier's registration is a pure bug, this should never happen.
>> If you know the way to reproduce this situation -- you need to fix it.
>>
>> 2nd registration can happen in 2 cases:
>> 1) missed rollback, when someone forget to call unregister after
>> successfull registration,
>> and then tried to call register again. It can lead to crash for example
>> when according module will be unloaded.
>> 2) some subsystem is registered twice, for example from different
>> namespaces.
>> in this case unregister called during sybsystem cleanup in first
>> namespace will incorrectly remove notifier used
>> in second namespace, it also can lead to unexpacted behaviour.
>>
> So in these two cases, is it more reasonable to trigger BUG() directly
> when checking for duplicate registration ?
> But why does current notifier_chain_register() just trigger WARN()
> without exiting ?
> notifier_chain_cond_register() direct exit without triggering WARN() ?
It should recover from this, if it can be detected. The main point is
that not all apis have to be this "robust" when used within the kernel
as we do allow for the callers to know what they are doing :)
>>> In the notifier_chain_register(), the condition ( (*nl) == n) is the same
>>> registration of the same hook.
>>> We can intercept this situation and avoid forming a linked list ring to
>>> make the API more rob
>>
>> Once again -- yes, you CAN prevent list corruption, but you CANNOT recover
>> the host and return it back to safe state.
>> If double register event was detected -- it means you have bug in kernel.
>>
>> Yes, you can add BUG here and crash the host immediately, but I prefer to
>> use warning in such situations.
>>
If this does not cause any additional problems or slow downs, it's
probably fine to add.
>>> Notifier_chain_register() is not a system hotspot function.
>>> At the same time, there is already a WARN_ONCE judgment. There is no new
>>> judgment in the new patch.
>>> It only changes the processing under the condition of (*nl) == n, which
>>> will not cause performance problems.
>>> At the same time, avoiding the formation of a link ring can make the system
>>> more robust.
>>
>> I disagree,
>> yes, node will have correct list, but anyway node will work wrong and can
>> crash the host in any time.
>
> Sorry, my description is not accurate.
>
> My patch feature does not prevent users from repeatedly registering hooks.
> But avoiding the chain ring caused by the user repeatedly registering the hook
>
> There are no modules for duplicate registration hooks in the current system.
> But considering that not all modules are in the kernel source tree,
> In order to improve the robustness of the kernel API, we
Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On 2019/7/15 13:38, Vasily Averin wrote:
> On 7/14/19 5:45 AM, Xiaoming Ni wrote:
>> On 2019/7/12 22:07, gre...@linuxfoundation.org wrote:
>>> On Fri, Jul 12, 2019 at 09:11:57PM +0800, Xiaoming Ni wrote:
On 2019/7/11 21:57, Vasily Averin wrote:
> On 7/11/19 4:55 AM, Nixiaoming wrote:
>> On Wed, July 10, 2019 1:49 PM Vasily Averin wrote:
>>> On 7/10/19 6:09 AM, Xiaoming Ni wrote:
Registering the same notifier to a hook repeatedly can cause the hook
list to form a ring or lose other members of the list.
>>>
>>> I think is not enough to _prevent_ 2nd register attempt,
>>> it's enough to detect just attempt and generate warning to mark host in
>>> bad state.
>>>
>>
>> Duplicate registration is prevented in my patch, not just "mark host in
>> bad state"
>>
>> Duplicate registration is checked and exited in
>> notifier_chain_cond_register()
>>
>> Duplicate registration was checked in notifier_chain_register() but only
>> the alarm was triggered without exiting. added by commit
>> 831246570d34692e
>> ("kernel/notifier.c: double register detection")
>>
>> My patch is like a combination of 831246570d34692e and
>> notifier_chain_cond_register(),
>> which triggers an alarm and exits when a duplicate registration is
>> detected.
>>
>>> Unexpected 2nd register of the same hook most likely will lead to 2nd
>>> unregister,
>>> and it can lead to host crash in any time:
>>> you can unregister notifier on first attempt it can be too early, it
>>> can be still in use.
>>> on the other hand you can never call 2nd unregister at all.
>>
>> Since the member was not added to the linked list at the time of the
>> second registration,
>> no linked list ring was formed.
>> The member is released on the first unregistration and -ENOENT on the
>> second unregistration.
>> After patching, the fault has been alleviated
>
> You are wrong here.
> 2nd notifier's registration is a pure bug, this should never happen.
> If you know the way to reproduce this situation -- you need to fix it.
>
> 2nd registration can happen in 2 cases:
> 1) missed rollback, when someone forget to call unregister after
> successfull registration,
> and then tried to call register again. It can lead to crash for example
> when according module will be unloaded.
> 2) some subsystem is registered twice, for example from different
> namespaces.
> in this case unregister called during sybsystem cleanup in first
> namespace will incorrectly remove notifier used
> in second namespace, it also can lead to unexpacted behaviour.
>
So in these two cases, is it more reasonable to trigger BUG() directly
when checking for duplicate registration ?
But why does current notifier_chain_register() just trigger WARN() without
exiting ?
notifier_chain_cond_register() direct exit without triggering WARN() ?
>>>
>>> It should recover from this, if it can be detected. The main point is
>>> that not all apis have to be this "robust" when used within the kernel
>>> as we do allow for the callers to know what they are doing :)
>>>
>> In the notifier_chain_register(), the condition ( (*nl) == n) is the same
>> registration of the same hook.
>> We can intercept this situation and avoid forming a linked list ring to
>> make the API more rob
>
> Once again -- yes, you CAN prevent list corruption, but you CANNOT recover
> the host and return it back to safe state.
> If double register event was detected -- it means you have bug in kernel.
>
> Yes, you can add BUG here and crash the host immediately, but I prefer to use
> warning in such situations.
>
>>> If this does not cause any additional problems or slow downs, it's
>>> probably fine to add.
>>>
>> Notifier_chain_register() is not a system hotspot function.
>> At the same time, there is already a WARN_ONCE judgment. There is no new
>> judgment in the new patch.
>> It only changes the processing under the condition of (*nl) == n, which will
>> not cause performance problems.
>> At the same time, avoiding the formation of a link ring can make the system
>> more robust.
>
> I disagree,
> yes, node will have correct list, but anyway node will work wrong and can
> crash the host in any time.
Sorry, my description is not accurate.
My patch feature does not prevent users from repeatedly registering hooks.
But avoiding the chain ring caused by the user repeatedly registering the hook
There are no modules for duplicate registration hooks in the current system.
But considering that not all modules are in the kernel source tree,
In order to improve the robustness of the kernel API, we should avoid the
linked list ring caused by repeated registration.
Or in order to improve the efficiency of problem location, when the duplicate
registration is
Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On 7/14/19 5:45 AM, Xiaoming Ni wrote:
> On 2019/7/12 22:07, gre...@linuxfoundation.org wrote:
>> On Fri, Jul 12, 2019 at 09:11:57PM +0800, Xiaoming Ni wrote:
>>> On 2019/7/11 21:57, Vasily Averin wrote:
On 7/11/19 4:55 AM, Nixiaoming wrote:
> On Wed, July 10, 2019 1:49 PM Vasily Averin wrote:
>> On 7/10/19 6:09 AM, Xiaoming Ni wrote:
>>> Registering the same notifier to a hook repeatedly can cause the hook
>>> list to form a ring or lose other members of the list.
>>
>> I think is not enough to _prevent_ 2nd register attempt,
>> it's enough to detect just attempt and generate warning to mark host in
>> bad state.
>>
>
> Duplicate registration is prevented in my patch, not just "mark host in
> bad state"
>
> Duplicate registration is checked and exited in
> notifier_chain_cond_register()
>
> Duplicate registration was checked in notifier_chain_register() but only
> the alarm was triggered without exiting. added by commit 831246570d34692e
> ("kernel/notifier.c: double register detection")
>
> My patch is like a combination of 831246570d34692e and
> notifier_chain_cond_register(),
> which triggers an alarm and exits when a duplicate registration is
> detected.
>
>> Unexpected 2nd register of the same hook most likely will lead to 2nd
>> unregister,
>> and it can lead to host crash in any time:
>> you can unregister notifier on first attempt it can be too early, it can
>> be still in use.
>> on the other hand you can never call 2nd unregister at all.
>
> Since the member was not added to the linked list at the time of the
> second registration,
> no linked list ring was formed.
> The member is released on the first unregistration and -ENOENT on the
> second unregistration.
> After patching, the fault has been alleviated
You are wrong here.
2nd notifier's registration is a pure bug, this should never happen.
If you know the way to reproduce this situation -- you need to fix it.
2nd registration can happen in 2 cases:
1) missed rollback, when someone forget to call unregister after
successfull registration,
and then tried to call register again. It can lead to crash for example
when according module will be unloaded.
2) some subsystem is registered twice, for example from different
namespaces.
in this case unregister called during sybsystem cleanup in first namespace
will incorrectly remove notifier used
in second namespace, it also can lead to unexpacted behaviour.
>>> So in these two cases, is it more reasonable to trigger BUG() directly when
>>> checking for duplicate registration ?
>>> But why does current notifier_chain_register() just trigger WARN() without
>>> exiting ?
>>> notifier_chain_cond_register() direct exit without triggering WARN() ?
>>
>> It should recover from this, if it can be detected. The main point is
>> that not all apis have to be this "robust" when used within the kernel
>> as we do allow for the callers to know what they are doing :)
>>
> In the notifier_chain_register(), the condition ( (*nl) == n) is the same
> registration of the same hook.
> We can intercept this situation and avoid forming a linked list ring to make
> the API more rob
Once again -- yes, you CAN prevent list corruption, but you CANNOT recover the
host and return it back to safe state.
If double register event was detected -- it means you have bug in kernel.
Yes, you can add BUG here and crash the host immediately, but I prefer to use
warning in such situations.
>> If this does not cause any additional problems or slow downs, it's
>> probably fine to add.
>>
> Notifier_chain_register() is not a system hotspot function.
> At the same time, there is already a WARN_ONCE judgment. There is no new
> judgment in the new patch.
> It only changes the processing under the condition of (*nl) == n, which will
> not cause performance problems.
> At the same time, avoiding the formation of a link ring can make the system
> more robust.
I disagree,
yes, node will have correct list, but anyway node will work wrong and can crash
the host in any time.
>> thanks,
>>
>> greg k-h
>>
>> .
>>
> Thanks
>
> Xiaoming Ni
>
>
>
Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On 2019/7/12 22:07, gre...@linuxfoundation.org wrote:
> On Fri, Jul 12, 2019 at 09:11:57PM +0800, Xiaoming Ni wrote:
>> On 2019/7/11 21:57, Vasily Averin wrote:
>>> On 7/11/19 4:55 AM, Nixiaoming wrote:
On Wed, July 10, 2019 1:49 PM Vasily Averin wrote:
> On 7/10/19 6:09 AM, Xiaoming Ni wrote:
>> Registering the same notifier to a hook repeatedly can cause the hook
>> list to form a ring or lose other members of the list.
>
> I think is not enough to _prevent_ 2nd register attempt,
> it's enough to detect just attempt and generate warning to mark host in
> bad state.
>
Duplicate registration is prevented in my patch, not just "mark host in
bad state"
Duplicate registration is checked and exited in
notifier_chain_cond_register()
Duplicate registration was checked in notifier_chain_register() but only
the alarm was triggered without exiting. added by commit 831246570d34692e
("kernel/notifier.c: double register detection")
My patch is like a combination of 831246570d34692e and
notifier_chain_cond_register(),
which triggers an alarm and exits when a duplicate registration is
detected.
> Unexpected 2nd register of the same hook most likely will lead to 2nd
> unregister,
> and it can lead to host crash in any time:
> you can unregister notifier on first attempt it can be too early, it can
> be still in use.
> on the other hand you can never call 2nd unregister at all.
Since the member was not added to the linked list at the time of the
second registration,
no linked list ring was formed.
The member is released on the first unregistration and -ENOENT on the
second unregistration.
After patching, the fault has been alleviated
>>>
>>> You are wrong here.
>>> 2nd notifier's registration is a pure bug, this should never happen.
>>> If you know the way to reproduce this situation -- you need to fix it.
>>>
>>> 2nd registration can happen in 2 cases:
>>> 1) missed rollback, when someone forget to call unregister after
>>> successfull registration,
>>> and then tried to call register again. It can lead to crash for example
>>> when according module will be unloaded.
>>> 2) some subsystem is registered twice, for example from different
>>> namespaces.
>>> in this case unregister called during sybsystem cleanup in first namespace
>>> will incorrectly remove notifier used
>>> in second namespace, it also can lead to unexpacted behaviour.
>>>
>> So in these two cases, is it more reasonable to trigger BUG() directly when
>> checking for duplicate registration ?
>> But why does current notifier_chain_register() just trigger WARN() without
>> exiting ?
>> notifier_chain_cond_register() direct exit without triggering WARN() ?
>
> It should recover from this, if it can be detected. The main point is
> that not all apis have to be this "robust" when used within the kernel
> as we do allow for the callers to know what they are doing :)
>
In the notifier_chain_register(), the condition ( (*nl) == n) is the same
registration of the same hook.
We can intercept this situation and avoid forming a linked list ring to make
the API more rob
> If this does not cause any additional problems or slow downs, it's
> probably fine to add.
>
Notifier_chain_register() is not a system hotspot function.
At the same time, there is already a WARN_ONCE judgment. There is no new
judgment in the new patch.
It only changes the processing under the condition of (*nl) == n, which will
not cause performance problems.
At the same time, avoiding the formation of a link ring can make the system
more robust.
> thanks,
>
> greg k-h
>
> .
>
Thanks
Xiaoming Ni
Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On Fri, Jul 12, 2019 at 09:11:57PM +0800, Xiaoming Ni wrote:
> On 2019/7/11 21:57, Vasily Averin wrote:
> > On 7/11/19 4:55 AM, Nixiaoming wrote:
> >> On Wed, July 10, 2019 1:49 PM Vasily Averin wrote:
> >>> On 7/10/19 6:09 AM, Xiaoming Ni wrote:
> Registering the same notifier to a hook repeatedly can cause the hook
> list to form a ring or lose other members of the list.
> >>>
> >>> I think is not enough to _prevent_ 2nd register attempt,
> >>> it's enough to detect just attempt and generate warning to mark host in
> >>> bad state.
> >>>
> >>
> >> Duplicate registration is prevented in my patch, not just "mark host in
> >> bad state"
> >>
> >> Duplicate registration is checked and exited in
> >> notifier_chain_cond_register()
> >>
> >> Duplicate registration was checked in notifier_chain_register() but only
> >> the alarm was triggered without exiting. added by commit 831246570d34692e
> >> ("kernel/notifier.c: double register detection")
> >>
> >> My patch is like a combination of 831246570d34692e and
> >> notifier_chain_cond_register(),
> >> which triggers an alarm and exits when a duplicate registration is
> >> detected.
> >>
> >>> Unexpected 2nd register of the same hook most likely will lead to 2nd
> >>> unregister,
> >>> and it can lead to host crash in any time:
> >>> you can unregister notifier on first attempt it can be too early, it can
> >>> be still in use.
> >>> on the other hand you can never call 2nd unregister at all.
> >>
> >> Since the member was not added to the linked list at the time of the
> >> second registration,
> >> no linked list ring was formed.
> >> The member is released on the first unregistration and -ENOENT on the
> >> second unregistration.
> >> After patching, the fault has been alleviated
> >
> > You are wrong here.
> > 2nd notifier's registration is a pure bug, this should never happen.
> > If you know the way to reproduce this situation -- you need to fix it.
> >
> > 2nd registration can happen in 2 cases:
> > 1) missed rollback, when someone forget to call unregister after
> > successfull registration,
> > and then tried to call register again. It can lead to crash for example
> > when according module will be unloaded.
> > 2) some subsystem is registered twice, for example from different
> > namespaces.
> > in this case unregister called during sybsystem cleanup in first namespace
> > will incorrectly remove notifier used
> > in second namespace, it also can lead to unexpacted behaviour.
> >
> So in these two cases, is it more reasonable to trigger BUG() directly when
> checking for duplicate registration ?
> But why does current notifier_chain_register() just trigger WARN() without
> exiting ?
> notifier_chain_cond_register() direct exit without triggering WARN() ?
It should recover from this, if it can be detected. The main point is
that not all apis have to be this "robust" when used within the kernel
as we do allow for the callers to know what they are doing :)
If this does not cause any additional problems or slow downs, it's
probably fine to add.
thanks,
greg k-h
Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On 2019/7/11 21:57, Vasily Averin wrote:
> On 7/11/19 4:55 AM, Nixiaoming wrote:
>> On Wed, July 10, 2019 1:49 PM Vasily Averin wrote:
>>> On 7/10/19 6:09 AM, Xiaoming Ni wrote:
Registering the same notifier to a hook repeatedly can cause the hook
list to form a ring or lose other members of the list.
>>>
>>> I think is not enough to _prevent_ 2nd register attempt,
>>> it's enough to detect just attempt and generate warning to mark host in bad
>>> state.
>>>
>>
>> Duplicate registration is prevented in my patch, not just "mark host in bad
>> state"
>>
>> Duplicate registration is checked and exited in
>> notifier_chain_cond_register()
>>
>> Duplicate registration was checked in notifier_chain_register() but only
>> the alarm was triggered without exiting. added by commit 831246570d34692e
>> ("kernel/notifier.c: double register detection")
>>
>> My patch is like a combination of 831246570d34692e and
>> notifier_chain_cond_register(),
>> which triggers an alarm and exits when a duplicate registration is detected.
>>
>>> Unexpected 2nd register of the same hook most likely will lead to 2nd
>>> unregister,
>>> and it can lead to host crash in any time:
>>> you can unregister notifier on first attempt it can be too early, it can be
>>> still in use.
>>> on the other hand you can never call 2nd unregister at all.
>>
>> Since the member was not added to the linked list at the time of the second
>> registration,
>> no linked list ring was formed.
>> The member is released on the first unregistration and -ENOENT on the second
>> unregistration.
>> After patching, the fault has been alleviated
>
> You are wrong here.
> 2nd notifier's registration is a pure bug, this should never happen.
> If you know the way to reproduce this situation -- you need to fix it.
>
> 2nd registration can happen in 2 cases:
> 1) missed rollback, when someone forget to call unregister after successfull
> registration,
> and then tried to call register again. It can lead to crash for example when
> according module will be unloaded.
> 2) some subsystem is registered twice, for example from different namespaces.
> in this case unregister called during sybsystem cleanup in first namespace
> will incorrectly remove notifier used
> in second namespace, it also can lead to unexpacted behaviour.
>
So in these two cases, is it more reasonable to trigger BUG() directly when
checking for duplicate registration ?
But why does current notifier_chain_register() just trigger WARN() without
exiting ?
notifier_chain_cond_register() direct exit without triggering WARN() ?
Thanks
Xiaoming Ni
>> It may be more helpful to return an error code when someone tries to
>> register the same
>> notification program a second time.
>
> You are wrong again here, it is senseless.
> If you have detected 2nd register -- your node is already in bad state.
>
>> But I noticed that notifier_chain_cond_register() returns 0 when duplicate
>> registration
>> is detected. At the same time, in all the existing export function comments
>> of notify,
>> "Currently always returns zero"
>>
>> I am a bit confused: which is better?
>>
>>>
>>> Unfortunately I do not see any ways to handle such cases properly,
>>> and it seems for me your patches does not resolve this problem.
>>>
>>> Am I missed something probably?
>>>
case1: An infinite loop in notifier_chain_register() can cause soft lockup
atomic_notifier_chain_register(_notifier_list, );
atomic_notifier_chain_register(_notifier_list, );
atomic_notifier_chain_register(_notifier_list, );
>>
>> Thanks
>>
>> Xiaoming Ni
>>
>
> .
>
Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On 7/11/19 4:55 AM, Nixiaoming wrote:
> On Wed, July 10, 2019 1:49 PM Vasily Averin wrote:
>> On 7/10/19 6:09 AM, Xiaoming Ni wrote:
>>> Registering the same notifier to a hook repeatedly can cause the hook
>>> list to form a ring or lose other members of the list.
>>
>> I think is not enough to _prevent_ 2nd register attempt,
>> it's enough to detect just attempt and generate warning to mark host in bad
>> state.
>>
>
> Duplicate registration is prevented in my patch, not just "mark host in bad
> state"
>
> Duplicate registration is checked and exited in notifier_chain_cond_register()
>
> Duplicate registration was checked in notifier_chain_register() but only
> the alarm was triggered without exiting. added by commit 831246570d34692e
> ("kernel/notifier.c: double register detection")
>
> My patch is like a combination of 831246570d34692e and
> notifier_chain_cond_register(),
> which triggers an alarm and exits when a duplicate registration is detected.
>
>> Unexpected 2nd register of the same hook most likely will lead to 2nd
>> unregister,
>> and it can lead to host crash in any time:
>> you can unregister notifier on first attempt it can be too early, it can be
>> still in use.
>> on the other hand you can never call 2nd unregister at all.
>
> Since the member was not added to the linked list at the time of the second
> registration,
> no linked list ring was formed.
> The member is released on the first unregistration and -ENOENT on the second
> unregistration.
> After patching, the fault has been alleviated
You are wrong here.
2nd notifier's registration is a pure bug, this should never happen.
If you know the way to reproduce this situation -- you need to fix it.
2nd registration can happen in 2 cases:
1) missed rollback, when someone forget to call unregister after successfull
registration,
and then tried to call register again. It can lead to crash for example when
according module will be unloaded.
2) some subsystem is registered twice, for example from different namespaces.
in this case unregister called during sybsystem cleanup in first namespace will
incorrectly remove notifier used
in second namespace, it also can lead to unexpacted behaviour.
> It may be more helpful to return an error code when someone tries to register
> the same
> notification program a second time.
You are wrong again here, it is senseless.
If you have detected 2nd register -- your node is already in bad state.
> But I noticed that notifier_chain_cond_register() returns 0 when duplicate
> registration
> is detected. At the same time, in all the existing export function comments
> of notify,
> "Currently always returns zero"
>
> I am a bit confused: which is better?
>
>>
>> Unfortunately I do not see any ways to handle such cases properly,
>> and it seems for me your patches does not resolve this problem.
>>
>> Am I missed something probably?
>>
>>> case1: An infinite loop in notifier_chain_register() can cause soft lockup
>>> atomic_notifier_chain_register(_notifier_list, );
>>> atomic_notifier_chain_register(_notifier_list, );
>>> atomic_notifier_chain_register(_notifier_list, );
>
> Thanks
>
> Xiaoming Ni
>
RE: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On Wed, July 10, 2019 1:49 PM Vasily Averin wrote:
>On 7/10/19 6:09 AM, Xiaoming Ni wrote:
>> Registering the same notifier to a hook repeatedly can cause the hook
>> list to form a ring or lose other members of the list.
>
>I think is not enough to _prevent_ 2nd register attempt,
>it's enough to detect just attempt and generate warning to mark host in bad
>state.
>
Duplicate registration is prevented in my patch, not just "mark host in bad
state"
Duplicate registration is checked and exited in notifier_chain_cond_register()
Duplicate registration was checked in notifier_chain_register() but only
the alarm was triggered without exiting. added by commit 831246570d34692e
("kernel/notifier.c: double register detection")
My patch is like a combination of 831246570d34692e and
notifier_chain_cond_register(),
which triggers an alarm and exits when a duplicate registration is detected.
>Unexpected 2nd register of the same hook most likely will lead to 2nd
>unregister,
>and it can lead to host crash in any time:
>you can unregister notifier on first attempt it can be too early, it can be
>still in use.
>on the other hand you can never call 2nd unregister at all.
Since the member was not added to the linked list at the time of the second
registration,
no linked list ring was formed.
The member is released on the first unregistration and -ENOENT on the second
unregistration.
After patching, the fault has been alleviated
It may be more helpful to return an error code when someone tries to register
the same
notification program a second time.
But I noticed that notifier_chain_cond_register() returns 0 when duplicate
registration
is detected. At the same time, in all the existing export function comments of
notify,
"Currently always returns zero"
I am a bit confused: which is better?
>
>Unfortunately I do not see any ways to handle such cases properly,
>and it seems for me your patches does not resolve this problem.
>
>Am I missed something probably?
>
>> case1: An infinite loop in notifier_chain_register() can cause soft lockup
>> atomic_notifier_chain_register(_notifier_list, );
>> atomic_notifier_chain_register(_notifier_list, );
>> atomic_notifier_chain_register(_notifier_list, );
Thanks
Xiaoming Ni
RE: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On Wed, July 10, 2019 1:56 PM Greg KH wrote:
>On Wed, Jul 10, 2019 at 11:09:07AM +0800, Xiaoming Ni wrote:
>> Registering the same notifier to a hook repeatedly can cause the hook
>> list to form a ring or lose other members of the list.
>
>Then don't do that :)
>
Duplicate registration is checked and exited in notifier_chain_cond_register()
Duplicate registration was checked in notifier_chain_register() but only
the alarm was triggered without exiting. added by commit 831246570d34692e
("kernel/notifier.c: double register detection")
This patch is similar to commit 8312465 and notifier_chain_cond_register(),
with actual prevention for such behaviour, which I think is necessary to
avoid the formation of a linked list ring.
>Is there any in-kernel users that do do this? If so, please just fix
>them.
>
Notifier_chain_register() is not a hotspot path.
Adding a check here can make the kernel more stable.
Thanks
Xiaoming Ni
>thanks,
>
>greg k-h
>
Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On Wed, Jul 10, 2019 at 11:09:07AM +0800, Xiaoming Ni wrote: > Registering the same notifier to a hook repeatedly can cause the hook > list to form a ring or lose other members of the list. Then don't do that :) Is there any in-kernel users that do do this? If so, please just fix them. thanks, greg k-h
Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On 7/10/19 6:09 AM, Xiaoming Ni wrote: > Registering the same notifier to a hook repeatedly can cause the hook > list to form a ring or lose other members of the list. I think is not enough to _prevent_ 2nd register attempt, it's enough to detect just attempt and generate warning to mark host in bad state. Unexpected 2nd register of the same hook most likely will lead to 2nd unregister, and it can lead to host crash in any time: you can unregister notifier on first attempt it can be too early, it can be still in use. on the other hand you can never call 2nd unregister at all. Unfortunately I do not see any ways to handle such cases properly, and it seems for me your patches does not resolve this problem. Am I missed something probably? > case1: An infinite loop in notifier_chain_register() can cause soft lockup > atomic_notifier_chain_register(_notifier_list, ); > atomic_notifier_chain_register(_notifier_list, ); > atomic_notifier_chain_register(_notifier_list, ); > > case2: An infinite loop in notifier_chain_register() can cause soft lockup > atomic_notifier_chain_register(_notifier_list, ); > atomic_notifier_chain_register(_notifier_list, ); > atomic_notifier_call_chain(_notifier_list, 0, NULL); > > case3: lose other hook test2 > atomic_notifier_chain_register(_notifier_list, ); > atomic_notifier_chain_register(_notifier_list, ); > atomic_notifier_chain_register(_notifier_list, ); > > case4: Unregister returns 0, but the hook is still in the linked list, > and it is not really registered. If you call notifier_call_chain > after ko is unloaded, it will trigger oops. if the system is > configured with softlockup_panic and the same hook is repeatedly > registered on the panic_notifier_list, it will cause a loop > panic. > > so. need add a check in in notifier_chain_register() to avoid duplicate > registration > > v1: > * use notifier_chain_cond_register replace notifier_chain_register > > v2: > * Add a check in notifier_chain_register() to avoid duplicate registration > * remove notifier_chain_cond_register() to avoid duplicate code > * remove blocking_notifier_chain_cond_register() to avoid duplicate code > > v3: > * Add a cover letter. > > Xiaoming Ni (3): > kernel/notifier.c: avoid duplicate registration > kernel/notifier.c: remove notifier_chain_cond_register() > kernel/notifier.c: remove blocking_notifier_chain_cond_register() > > include/linux/notifier.h | 4 > kernel/notifier.c| 41 +++-- > net/sunrpc/rpc_pipe.c| 2 +- > 3 files changed, 4 insertions(+), 43 deletions(-) >
