diff mbox series

[v4,05/25] reboot: Warn if restart handler has duplicated priority

Message ID 20211126180101.27818-6-digetx@gmail.com (mailing list archive)
State New, archived
Headers show
Series Introduce power-off+restart call chain API | expand

Commit Message

Dmitry Osipenko Nov. 26, 2021, 6 p.m. UTC
Add sanity check which ensures that there are no two restart handlers
registered with the same priority. Normally it's a direct sign of a
problem if two handlers use the same priority.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 kernel/reboot.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Michał Mirosław Nov. 28, 2021, 12:28 a.m. UTC | #1
On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
> Add sanity check which ensures that there are no two restart handlers
> registered with the same priority. Normally it's a direct sign of a
> problem if two handlers use the same priority.

The patch doesn't ensure the property that there are no duplicated-priority
entries on the chain.

I'd rather see a atomic_notifier_chain_register_unique() that returns
-EBUSY or something istead of adding an entry with duplicate priority.
That way it would need only one list traversal unless you want to
register the duplicate anyway (then you would call the older
atomic_notifier_chain_register() after reporting the error).

(Or you could return > 0 when a duplicate is registered in
atomic_notifier_chain_register() if the callers are prepared
for that. I don't really like this way, though.)

Best Regards
Michał Mirosław
Dmitry Osipenko Nov. 28, 2021, 9:06 p.m. UTC | #2
28.11.2021 03:28, Michał Mirosław пишет:
> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
>> Add sanity check which ensures that there are no two restart handlers
>> registered with the same priority. Normally it's a direct sign of a
>> problem if two handlers use the same priority.
> 
> The patch doesn't ensure the property that there are no duplicated-priority
> entries on the chain.

It's not the exact point of this patch.

> I'd rather see a atomic_notifier_chain_register_unique() that returns
> -EBUSY or something istead of adding an entry with duplicate priority.
> That way it would need only one list traversal unless you want to
> register the duplicate anyway (then you would call the older
> atomic_notifier_chain_register() after reporting the error).

The point of this patch is to warn developers about the problem that
needs to be fixed. We already have such troubling drivers in mainline.

It's not critical to register different handlers with a duplicated
priorities, but such cases really need to be corrected. We shouldn't
break users' machines during transition to the new API, meanwhile
developers should take action of fixing theirs drivers.

> (Or you could return > 0 when a duplicate is registered in
> atomic_notifier_chain_register() if the callers are prepared
> for that. I don't really like this way, though.)

I had a similar thought at some point before and decided that I'm not in
favor of this approach. It's nicer to have a dedicated function that
verifies the uniqueness, IMO.
Michał Mirosław Nov. 29, 2021, 12:26 a.m. UTC | #3
On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
> 28.11.2021 03:28, Michał Mirosław пишет:
> > On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
> >> Add sanity check which ensures that there are no two restart handlers
> >> registered with the same priority. Normally it's a direct sign of a
> >> problem if two handlers use the same priority.
> > 
> > The patch doesn't ensure the property that there are no duplicated-priority
> > entries on the chain.
> 
> It's not the exact point of this patch.
> 
> > I'd rather see a atomic_notifier_chain_register_unique() that returns
> > -EBUSY or something istead of adding an entry with duplicate priority.
> > That way it would need only one list traversal unless you want to
> > register the duplicate anyway (then you would call the older
> > atomic_notifier_chain_register() after reporting the error).
> 
> The point of this patch is to warn developers about the problem that
> needs to be fixed. We already have such troubling drivers in mainline.
> 
> It's not critical to register different handlers with a duplicated
> priorities, but such cases really need to be corrected. We shouldn't
> break users' machines during transition to the new API, meanwhile
> developers should take action of fixing theirs drivers.
> 
> > (Or you could return > 0 when a duplicate is registered in
> > atomic_notifier_chain_register() if the callers are prepared
> > for that. I don't really like this way, though.)
> 
> I had a similar thought at some point before and decided that I'm not in
> favor of this approach. It's nicer to have a dedicated function that
> verifies the uniqueness, IMO.

I don't like the part that it traverses the list second time to check
the uniqueness. But actually you could avoid that if
notifier_chain_register() would always add equal-priority entries in
reverse order:

 static int notifier_chain_register(struct notifier_block **nl,
 		struct notifier_block *n)
 {
 	while ((*nl) != NULL) {
 		if (unlikely((*nl) == n)) {
 			WARN(1, "double register detected");
 			return 0;
 		}
-		if (n->priority > (*nl)->priority)
+		if (n->priority >= (*nl)->priority)
 			break;
 		nl = &((*nl)->next);
 	}
 	n->next = *nl;
 	rcu_assign_pointer(*nl, n);
 	return 0;
 }

Then the check for uniqueness after adding would be:

 WARN(nb->next && nb->priority == nb->next->priority);

Best Regards
Michał Mirosław
Dmitry Osipenko Nov. 29, 2021, 11:34 a.m. UTC | #4
29.11.2021 03:26, Michał Mirosław пишет:
> On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
>> 28.11.2021 03:28, Michał Mirosław пишет:
>>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
>>>> Add sanity check which ensures that there are no two restart handlers
>>>> registered with the same priority. Normally it's a direct sign of a
>>>> problem if two handlers use the same priority.
>>>
>>> The patch doesn't ensure the property that there are no duplicated-priority
>>> entries on the chain.
>>
>> It's not the exact point of this patch.
>>
>>> I'd rather see a atomic_notifier_chain_register_unique() that returns
>>> -EBUSY or something istead of adding an entry with duplicate priority.
>>> That way it would need only one list traversal unless you want to
>>> register the duplicate anyway (then you would call the older
>>> atomic_notifier_chain_register() after reporting the error).
>>
>> The point of this patch is to warn developers about the problem that
>> needs to be fixed. We already have such troubling drivers in mainline.
>>
>> It's not critical to register different handlers with a duplicated
>> priorities, but such cases really need to be corrected. We shouldn't
>> break users' machines during transition to the new API, meanwhile
>> developers should take action of fixing theirs drivers.
>>
>>> (Or you could return > 0 when a duplicate is registered in
>>> atomic_notifier_chain_register() if the callers are prepared
>>> for that. I don't really like this way, though.)
>>
>> I had a similar thought at some point before and decided that I'm not in
>> favor of this approach. It's nicer to have a dedicated function that
>> verifies the uniqueness, IMO.
> 
> I don't like the part that it traverses the list second time to check
> the uniqueness. But actually you could avoid that if
> notifier_chain_register() would always add equal-priority entries in
> reverse order:
> 
>  static int notifier_chain_register(struct notifier_block **nl,
>  		struct notifier_block *n)
>  {
>  	while ((*nl) != NULL) {
>  		if (unlikely((*nl) == n)) {
>  			WARN(1, "double register detected");
>  			return 0;
>  		}
> -		if (n->priority > (*nl)->priority)
> +		if (n->priority >= (*nl)->priority)
>  			break;
>  		nl = &((*nl)->next);
>  	}
>  	n->next = *nl;
>  	rcu_assign_pointer(*nl, n);
>  	return 0;
>  }
> 
> Then the check for uniqueness after adding would be:
> 
>  WARN(nb->next && nb->priority == nb->next->priority);

We can't just change the registration order because invocation order of
the call chain depends on the registration order and some of current
users may rely on that order. I'm pretty sure that changing the order
will have unfortunate consequences.
Rafael J. Wysocki Dec. 10, 2021, 6:27 p.m. UTC | #5
On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 29.11.2021 03:26, Michał Mirosław пишет:
> > On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
> >> 28.11.2021 03:28, Michał Mirosław пишет:
> >>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
> >>>> Add sanity check which ensures that there are no two restart handlers
> >>>> registered with the same priority. Normally it's a direct sign of a
> >>>> problem if two handlers use the same priority.
> >>>
> >>> The patch doesn't ensure the property that there are no duplicated-priority
> >>> entries on the chain.
> >>
> >> It's not the exact point of this patch.
> >>
> >>> I'd rather see a atomic_notifier_chain_register_unique() that returns
> >>> -EBUSY or something istead of adding an entry with duplicate priority.
> >>> That way it would need only one list traversal unless you want to
> >>> register the duplicate anyway (then you would call the older
> >>> atomic_notifier_chain_register() after reporting the error).
> >>
> >> The point of this patch is to warn developers about the problem that
> >> needs to be fixed. We already have such troubling drivers in mainline.
> >>
> >> It's not critical to register different handlers with a duplicated
> >> priorities, but such cases really need to be corrected. We shouldn't
> >> break users' machines during transition to the new API, meanwhile
> >> developers should take action of fixing theirs drivers.
> >>
> >>> (Or you could return > 0 when a duplicate is registered in
> >>> atomic_notifier_chain_register() if the callers are prepared
> >>> for that. I don't really like this way, though.)
> >>
> >> I had a similar thought at some point before and decided that I'm not in
> >> favor of this approach. It's nicer to have a dedicated function that
> >> verifies the uniqueness, IMO.
> >
> > I don't like the part that it traverses the list second time to check
> > the uniqueness. But actually you could avoid that if
> > notifier_chain_register() would always add equal-priority entries in
> > reverse order:
> >
> >  static int notifier_chain_register(struct notifier_block **nl,
> >               struct notifier_block *n)
> >  {
> >       while ((*nl) != NULL) {
> >               if (unlikely((*nl) == n)) {
> >                       WARN(1, "double register detected");
> >                       return 0;
> >               }
> > -             if (n->priority > (*nl)->priority)
> > +             if (n->priority >= (*nl)->priority)
> >                       break;
> >               nl = &((*nl)->next);
> >       }
> >       n->next = *nl;
> >       rcu_assign_pointer(*nl, n);
> >       return 0;
> >  }
> >
> > Then the check for uniqueness after adding would be:
> >
> >  WARN(nb->next && nb->priority == nb->next->priority);
>
> We can't just change the registration order because invocation order of
> the call chain depends on the registration order

It doesn't if unique priorities are required and isn't that what you want?

> and some of current
> users may rely on that order. I'm pretty sure that changing the order
> will have unfortunate consequences.

Well, the WARN() doesn't help much then.

Either you can make all of the users register with unique priorities,
and then you can make the registration reject non-unique ones, or you
cannot assume them to be unique.
Dmitry Osipenko Dec. 10, 2021, 7:04 p.m. UTC | #6
10.12.2021 21:27, Rafael J. Wysocki пишет:
> On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 29.11.2021 03:26, Michał Mirosław пишет:
>>> On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
>>>> 28.11.2021 03:28, Michał Mirosław пишет:
>>>>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
>>>>>> Add sanity check which ensures that there are no two restart handlers
>>>>>> registered with the same priority. Normally it's a direct sign of a
>>>>>> problem if two handlers use the same priority.
>>>>>
>>>>> The patch doesn't ensure the property that there are no duplicated-priority
>>>>> entries on the chain.
>>>>
>>>> It's not the exact point of this patch.
>>>>
>>>>> I'd rather see a atomic_notifier_chain_register_unique() that returns
>>>>> -EBUSY or something istead of adding an entry with duplicate priority.
>>>>> That way it would need only one list traversal unless you want to
>>>>> register the duplicate anyway (then you would call the older
>>>>> atomic_notifier_chain_register() after reporting the error).
>>>>
>>>> The point of this patch is to warn developers about the problem that
>>>> needs to be fixed. We already have such troubling drivers in mainline.
>>>>
>>>> It's not critical to register different handlers with a duplicated
>>>> priorities, but such cases really need to be corrected. We shouldn't
>>>> break users' machines during transition to the new API, meanwhile
>>>> developers should take action of fixing theirs drivers.
>>>>
>>>>> (Or you could return > 0 when a duplicate is registered in
>>>>> atomic_notifier_chain_register() if the callers are prepared
>>>>> for that. I don't really like this way, though.)
>>>>
>>>> I had a similar thought at some point before and decided that I'm not in
>>>> favor of this approach. It's nicer to have a dedicated function that
>>>> verifies the uniqueness, IMO.
>>>
>>> I don't like the part that it traverses the list second time to check
>>> the uniqueness. But actually you could avoid that if
>>> notifier_chain_register() would always add equal-priority entries in
>>> reverse order:
>>>
>>>  static int notifier_chain_register(struct notifier_block **nl,
>>>               struct notifier_block *n)
>>>  {
>>>       while ((*nl) != NULL) {
>>>               if (unlikely((*nl) == n)) {
>>>                       WARN(1, "double register detected");
>>>                       return 0;
>>>               }
>>> -             if (n->priority > (*nl)->priority)
>>> +             if (n->priority >= (*nl)->priority)
>>>                       break;
>>>               nl = &((*nl)->next);
>>>       }
>>>       n->next = *nl;
>>>       rcu_assign_pointer(*nl, n);
>>>       return 0;
>>>  }
>>>
>>> Then the check for uniqueness after adding would be:
>>>
>>>  WARN(nb->next && nb->priority == nb->next->priority);
>>
>> We can't just change the registration order because invocation order of
>> the call chain depends on the registration order
> 
> It doesn't if unique priorities are required and isn't that what you want?
> 
>> and some of current
>> users may rely on that order. I'm pretty sure that changing the order
>> will have unfortunate consequences.
> 
> Well, the WARN() doesn't help much then.
> 
> Either you can make all of the users register with unique priorities,
> and then you can make the registration reject non-unique ones, or you
> cannot assume them to be unique.

There is no strong requirement for priorities to be unique, the reboot.c
code will work properly.

The potential problem is on the user's side and the warning is intended
to aid the user.

We can make it a strong requirement, but only after converting and
testing all kernel drivers. I'll consider to add patches for that.
Rafael J. Wysocki Dec. 10, 2021, 7:14 p.m. UTC | #7
On Fri, Dec 10, 2021 at 8:04 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 10.12.2021 21:27, Rafael J. Wysocki пишет:
> > On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> 29.11.2021 03:26, Michał Mirosław пишет:
> >>> On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
> >>>> 28.11.2021 03:28, Michał Mirosław пишет:
> >>>>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
> >>>>>> Add sanity check which ensures that there are no two restart handlers
> >>>>>> registered with the same priority. Normally it's a direct sign of a
> >>>>>> problem if two handlers use the same priority.
> >>>>>
> >>>>> The patch doesn't ensure the property that there are no duplicated-priority
> >>>>> entries on the chain.
> >>>>
> >>>> It's not the exact point of this patch.
> >>>>
> >>>>> I'd rather see a atomic_notifier_chain_register_unique() that returns
> >>>>> -EBUSY or something istead of adding an entry with duplicate priority.
> >>>>> That way it would need only one list traversal unless you want to
> >>>>> register the duplicate anyway (then you would call the older
> >>>>> atomic_notifier_chain_register() after reporting the error).
> >>>>
> >>>> The point of this patch is to warn developers about the problem that
> >>>> needs to be fixed. We already have such troubling drivers in mainline.
> >>>>
> >>>> It's not critical to register different handlers with a duplicated
> >>>> priorities, but such cases really need to be corrected. We shouldn't
> >>>> break users' machines during transition to the new API, meanwhile
> >>>> developers should take action of fixing theirs drivers.
> >>>>
> >>>>> (Or you could return > 0 when a duplicate is registered in
> >>>>> atomic_notifier_chain_register() if the callers are prepared
> >>>>> for that. I don't really like this way, though.)
> >>>>
> >>>> I had a similar thought at some point before and decided that I'm not in
> >>>> favor of this approach. It's nicer to have a dedicated function that
> >>>> verifies the uniqueness, IMO.
> >>>
> >>> I don't like the part that it traverses the list second time to check
> >>> the uniqueness. But actually you could avoid that if
> >>> notifier_chain_register() would always add equal-priority entries in
> >>> reverse order:
> >>>
> >>>  static int notifier_chain_register(struct notifier_block **nl,
> >>>               struct notifier_block *n)
> >>>  {
> >>>       while ((*nl) != NULL) {
> >>>               if (unlikely((*nl) == n)) {
> >>>                       WARN(1, "double register detected");
> >>>                       return 0;
> >>>               }
> >>> -             if (n->priority > (*nl)->priority)
> >>> +             if (n->priority >= (*nl)->priority)
> >>>                       break;
> >>>               nl = &((*nl)->next);
> >>>       }
> >>>       n->next = *nl;
> >>>       rcu_assign_pointer(*nl, n);
> >>>       return 0;
> >>>  }
> >>>
> >>> Then the check for uniqueness after adding would be:
> >>>
> >>>  WARN(nb->next && nb->priority == nb->next->priority);
> >>
> >> We can't just change the registration order because invocation order of
> >> the call chain depends on the registration order
> >
> > It doesn't if unique priorities are required and isn't that what you want?
> >
> >> and some of current
> >> users may rely on that order. I'm pretty sure that changing the order
> >> will have unfortunate consequences.
> >
> > Well, the WARN() doesn't help much then.
> >
> > Either you can make all of the users register with unique priorities,
> > and then you can make the registration reject non-unique ones, or you
> > cannot assume them to be unique.
>
> There is no strong requirement for priorities to be unique, the reboot.c
> code will work properly.

In which case adding the WARN() is not appropriate IMV.

Also I've looked at the existing code and at least in some cases the
order in which the notifiers run doesn't matter.  I'm not sure what
the purpose of this patch is TBH.

> The potential problem is on the user's side and the warning is intended
> to aid the user.

Unless somebody has the panic_on_warn mentioned previously set and
really the user need not understand what the WARN() is about.  IOW,
WARN() helps developers, not users.

> We can make it a strong requirement, but only after converting and
> testing all kernel drivers.

Right.

> I'll consider to add patches for that.

But can you avoid adding more patches to this series?
Dmitry Osipenko Dec. 10, 2021, 7:42 p.m. UTC | #8
10.12.2021 22:14, Rafael J. Wysocki пишет:
> On Fri, Dec 10, 2021 at 8:04 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 10.12.2021 21:27, Rafael J. Wysocki пишет:
>>> On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>
>>>> 29.11.2021 03:26, Michał Mirosław пишет:
>>>>> On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
>>>>>> 28.11.2021 03:28, Michał Mirosław пишет:
>>>>>>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
>>>>>>>> Add sanity check which ensures that there are no two restart handlers
>>>>>>>> registered with the same priority. Normally it's a direct sign of a
>>>>>>>> problem if two handlers use the same priority.
>>>>>>>
>>>>>>> The patch doesn't ensure the property that there are no duplicated-priority
>>>>>>> entries on the chain.
>>>>>>
>>>>>> It's not the exact point of this patch.
>>>>>>
>>>>>>> I'd rather see a atomic_notifier_chain_register_unique() that returns
>>>>>>> -EBUSY or something istead of adding an entry with duplicate priority.
>>>>>>> That way it would need only one list traversal unless you want to
>>>>>>> register the duplicate anyway (then you would call the older
>>>>>>> atomic_notifier_chain_register() after reporting the error).
>>>>>>
>>>>>> The point of this patch is to warn developers about the problem that
>>>>>> needs to be fixed. We already have such troubling drivers in mainline.
>>>>>>
>>>>>> It's not critical to register different handlers with a duplicated
>>>>>> priorities, but such cases really need to be corrected. We shouldn't
>>>>>> break users' machines during transition to the new API, meanwhile
>>>>>> developers should take action of fixing theirs drivers.
>>>>>>
>>>>>>> (Or you could return > 0 when a duplicate is registered in
>>>>>>> atomic_notifier_chain_register() if the callers are prepared
>>>>>>> for that. I don't really like this way, though.)
>>>>>>
>>>>>> I had a similar thought at some point before and decided that I'm not in
>>>>>> favor of this approach. It's nicer to have a dedicated function that
>>>>>> verifies the uniqueness, IMO.
>>>>>
>>>>> I don't like the part that it traverses the list second time to check
>>>>> the uniqueness. But actually you could avoid that if
>>>>> notifier_chain_register() would always add equal-priority entries in
>>>>> reverse order:
>>>>>
>>>>>  static int notifier_chain_register(struct notifier_block **nl,
>>>>>               struct notifier_block *n)
>>>>>  {
>>>>>       while ((*nl) != NULL) {
>>>>>               if (unlikely((*nl) == n)) {
>>>>>                       WARN(1, "double register detected");
>>>>>                       return 0;
>>>>>               }
>>>>> -             if (n->priority > (*nl)->priority)
>>>>> +             if (n->priority >= (*nl)->priority)
>>>>>                       break;
>>>>>               nl = &((*nl)->next);
>>>>>       }
>>>>>       n->next = *nl;
>>>>>       rcu_assign_pointer(*nl, n);
>>>>>       return 0;
>>>>>  }
>>>>>
>>>>> Then the check for uniqueness after adding would be:
>>>>>
>>>>>  WARN(nb->next && nb->priority == nb->next->priority);
>>>>
>>>> We can't just change the registration order because invocation order of
>>>> the call chain depends on the registration order
>>>
>>> It doesn't if unique priorities are required and isn't that what you want?
>>>
>>>> and some of current
>>>> users may rely on that order. I'm pretty sure that changing the order
>>>> will have unfortunate consequences.
>>>
>>> Well, the WARN() doesn't help much then.
>>>
>>> Either you can make all of the users register with unique priorities,
>>> and then you can make the registration reject non-unique ones, or you
>>> cannot assume them to be unique.
>>
>> There is no strong requirement for priorities to be unique, the reboot.c
>> code will work properly.
> 
> In which case adding the WARN() is not appropriate IMV.
> 
> Also I've looked at the existing code and at least in some cases the
> order in which the notifiers run doesn't matter.  I'm not sure what
> the purpose of this patch is TBH.

The purpose is to let developer know that driver needs to be corrected.

>> The potential problem is on the user's side and the warning is intended
>> to aid the user.
> 
> Unless somebody has the panic_on_warn mentioned previously set and
> really the user need not understand what the WARN() is about.  IOW,
> WARN() helps developers, not users.
> 
>> We can make it a strong requirement, but only after converting and
>> testing all kernel drivers.
> 
> Right.
> 
>> I'll consider to add patches for that.
> 
> But can you avoid adding more patches to this series?

I won't add more patches since such patches can be added only after
completion of transition to the new API of the whole kernel.
Dmitry Osipenko Dec. 10, 2021, 7:44 p.m. UTC | #9
10.12.2021 22:42, Dmitry Osipenko пишет:
...
>>> There is no strong requirement for priorities to be unique, the reboot.c
>>> code will work properly.
>>
>> In which case adding the WARN() is not appropriate IMV.
>>
>> Also I've looked at the existing code and at least in some cases the
>> order in which the notifiers run doesn't matter.  I'm not sure what
>> the purpose of this patch is TBH.
> 
> The purpose is to let developer know that driver needs to be corrected.
> 
>>> The potential problem is on the user's side and the warning is intended
>>> to aid the user.
>>
>> Unless somebody has the panic_on_warn mentioned previously set and
>> really the user need not understand what the WARN() is about.  IOW,
>> WARN() helps developers, not users.
>>
>>> We can make it a strong requirement, but only after converting and
>>> testing all kernel drivers.
>>
>> Right.
>>
>>> I'll consider to add patches for that.
>>
>> But can you avoid adding more patches to this series?
> 
> I won't add more patches since such patches can be added only after
> completion of transition to the new API of the whole kernel.
> 

Thank you for the review.
Dmitry Osipenko Dec. 10, 2021, 7:49 p.m. UTC | #10
10.12.2021 22:44, Dmitry Osipenko пишет:
> 10.12.2021 22:42, Dmitry Osipenko пишет:
> ...
>>>> There is no strong requirement for priorities to be unique, the reboot.c
>>>> code will work properly.
>>>
>>> In which case adding the WARN() is not appropriate IMV.
>>>
>>> Also I've looked at the existing code and at least in some cases the
>>> order in which the notifiers run doesn't matter.  I'm not sure what
>>> the purpose of this patch is TBH.
>>
>> The purpose is to let developer know that driver needs to be corrected.
>>
>>>> The potential problem is on the user's side and the warning is intended
>>>> to aid the user.
>>>
>>> Unless somebody has the panic_on_warn mentioned previously set and
>>> really the user need not understand what the WARN() is about.  IOW,
>>> WARN() helps developers, not users.
>>>
>>>> We can make it a strong requirement, but only after converting and
>>>> testing all kernel drivers.
>>>
>>> Right.
>>>
>>>> I'll consider to add patches for that.
>>>
>>> But can you avoid adding more patches to this series?
>>
>> I won't add more patches since such patches can be added only after
>> completion of transition to the new API of the whole kernel.
>>
> 
> Thank you for the review.
> 

I meant you, Rafael, and Michał, just in case :)
Geert Uytterhoeven Dec. 13, 2021, 9:23 a.m. UTC | #11
On Fri, Dec 10, 2021 at 8:14 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Dec 10, 2021 at 8:04 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> > 10.12.2021 21:27, Rafael J. Wysocki пишет:
> > > On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> > >> 29.11.2021 03:26, Michał Mirosław пишет:
> > >>> On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
> > >>>> 28.11.2021 03:28, Michał Mirosław пишет:
> > >>>>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
> > >>>>>> Add sanity check which ensures that there are no two restart handlers
> > >>>>>> registered with the same priority. Normally it's a direct sign of a
> > >>>>>> problem if two handlers use the same priority.
> > >>>>>
> > >>>>> The patch doesn't ensure the property that there are no duplicated-priority
> > >>>>> entries on the chain.
> > >>>>
> > >>>> It's not the exact point of this patch.
> > >>>>
> > >>>>> I'd rather see a atomic_notifier_chain_register_unique() that returns
> > >>>>> -EBUSY or something istead of adding an entry with duplicate priority.
> > >>>>> That way it would need only one list traversal unless you want to
> > >>>>> register the duplicate anyway (then you would call the older
> > >>>>> atomic_notifier_chain_register() after reporting the error).
> > >>>>
> > >>>> The point of this patch is to warn developers about the problem that
> > >>>> needs to be fixed. We already have such troubling drivers in mainline.
> > >>>>
> > >>>> It's not critical to register different handlers with a duplicated
> > >>>> priorities, but such cases really need to be corrected. We shouldn't
> > >>>> break users' machines during transition to the new API, meanwhile
> > >>>> developers should take action of fixing theirs drivers.
> > >>>>
> > >>>>> (Or you could return > 0 when a duplicate is registered in
> > >>>>> atomic_notifier_chain_register() if the callers are prepared
> > >>>>> for that. I don't really like this way, though.)
> > >>>>
> > >>>> I had a similar thought at some point before and decided that I'm not in
> > >>>> favor of this approach. It's nicer to have a dedicated function that
> > >>>> verifies the uniqueness, IMO.
> > >>>
> > >>> I don't like the part that it traverses the list second time to check
> > >>> the uniqueness. But actually you could avoid that if
> > >>> notifier_chain_register() would always add equal-priority entries in
> > >>> reverse order:
> > >>>
> > >>>  static int notifier_chain_register(struct notifier_block **nl,
> > >>>               struct notifier_block *n)
> > >>>  {
> > >>>       while ((*nl) != NULL) {
> > >>>               if (unlikely((*nl) == n)) {
> > >>>                       WARN(1, "double register detected");
> > >>>                       return 0;
> > >>>               }
> > >>> -             if (n->priority > (*nl)->priority)
> > >>> +             if (n->priority >= (*nl)->priority)
> > >>>                       break;
> > >>>               nl = &((*nl)->next);
> > >>>       }
> > >>>       n->next = *nl;
> > >>>       rcu_assign_pointer(*nl, n);
> > >>>       return 0;
> > >>>  }
> > >>>
> > >>> Then the check for uniqueness after adding would be:
> > >>>
> > >>>  WARN(nb->next && nb->priority == nb->next->priority);
> > >>
> > >> We can't just change the registration order because invocation order of
> > >> the call chain depends on the registration order
> > >
> > > It doesn't if unique priorities are required and isn't that what you want?
> > >
> > >> and some of current
> > >> users may rely on that order. I'm pretty sure that changing the order
> > >> will have unfortunate consequences.
> > >
> > > Well, the WARN() doesn't help much then.
> > >
> > > Either you can make all of the users register with unique priorities,
> > > and then you can make the registration reject non-unique ones, or you
> > > cannot assume them to be unique.
> >
> > There is no strong requirement for priorities to be unique, the reboot.c
> > code will work properly.
>
> In which case adding the WARN() is not appropriate IMV.
>
> Also I've looked at the existing code and at least in some cases the
> order in which the notifiers run doesn't matter.  I'm not sure what
> the purpose of this patch is TBH.
>
> > The potential problem is on the user's side and the warning is intended
> > to aid the user.
>
> Unless somebody has the panic_on_warn mentioned previously set and
> really the user need not understand what the WARN() is about.  IOW,
> WARN() helps developers, not users.

Do panic_on_warn and reboot_on_panic play well with having a WARN()
in the reboot notifier handling?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
diff mbox series

Patch

diff --git a/kernel/reboot.c b/kernel/reboot.c
index 6bcc5d6a6572..e6659ae329f1 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -182,7 +182,20 @@  static ATOMIC_NOTIFIER_HEAD(restart_handler_list);
  */
 int register_restart_handler(struct notifier_block *nb)
 {
-	return atomic_notifier_chain_register(&restart_handler_list, nb);
+	int ret;
+
+	ret = atomic_notifier_chain_register(&restart_handler_list, nb);
+	if (ret)
+		return ret;
+
+	/*
+	 * Handler must have unique priority. Otherwise call order is
+	 * determined by registration order, which is unreliable.
+	 */
+	WARN(!atomic_notifier_has_unique_priority(&restart_handler_list, nb),
+	     "restart handler must have unique priority\n");
+
+	return 0;
 }
 EXPORT_SYMBOL(register_restart_handler);