[PATCH] netlink: Allow command handler to use l_netlink_cancel

[PATCH] netlink: Allow command handler to use l_netlink_cancel

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

---
 ell/netlink.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/ell/netlink.c b/ell/netlink.c
index 33d0e5b..4664762 100644
--- a/ell/netlink.c
+++ b/ell/netlink.c
@@ -162,12 +162,15 @@ static void process_message(struct l_netlink *netlink, struct nlmsghdr *nlmsg)
 {
 	const void *data = nlmsg;
 	struct command *command;
+	uint32_t id;
 
 	command = l_hashmap_remove(netlink->command_pending,
 					L_UINT_TO_PTR(nlmsg->nlmsg_seq));
 	if (!command)
 		return;
 
+	id = command->id;
+
 	if (!command->handler)
 		goto done;
 
@@ -189,9 +192,8 @@ static void process_message(struct l_netlink *netlink, struct nlmsghdr *nlmsg)
 	}
 
 done:
-	l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(command->id));
-
-	destroy_command(command);
+	if (l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(id)))
+		destroy_command(command);
 }
 
 static void process_multi(struct l_netlink *netlink, struct nlmsghdr *nlmsg)
-- 
2.27.0

Re: [PATCH] netlink: Allow command handler to use l_netlink_cancel

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1442 bytes --]

Hi Andrew,

On 3/2/21 5:14 AM, Andrew Zaborowski wrote:
> ---
>   ell/netlink.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 

You may want to write something about why this would be wanted.

> diff --git a/ell/netlink.c b/ell/netlink.c
> index 33d0e5b..4664762 100644
> --- a/ell/netlink.c
> +++ b/ell/netlink.c
> @@ -162,12 +162,15 @@ static void process_message(struct l_netlink *netlink, struct nlmsghdr *nlmsg)
>   {
>   	const void *data = nlmsg;
>   	struct command *command;
> +	uint32_t id;
>   
>   	command = l_hashmap_remove(netlink->command_pending,
>   					L_UINT_TO_PTR(nlmsg->nlmsg_seq));
>   	if (!command)
>   		return;
>   
> +	id = command->id;
> +
>   	if (!command->handler)
>   		goto done;
>   
> @@ -189,9 +192,8 @@ static void process_message(struct l_netlink *netlink, struct nlmsghdr *nlmsg)
>   	}
>   
>   done:
> -	l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(command->id));
> -
> -	destroy_command(command);
> +	if (l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(id)))
> +		destroy_command(command);

Would it not be better to simply remove the command from the command_lookup 
hashmap prior to dispatch, and have l_netlink_cancel simply fail?

I do wonder why you'd cancel the command from the handler?

>   }
>   
>   static void process_multi(struct l_netlink *netlink, struct nlmsghdr *nlmsg)
> 

Regards,
-Denis

Re: [PATCH] netlink: Allow command handler to use l_netlink_cancel

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1903 bytes --]

On Tue, 2 Mar 2021 at 16:10, Denis Kenzior <denkenz@gmail.com> wrote:
> On 3/2/21 5:14 AM, Andrew Zaborowski wrote:
> > ---
> >   ell/netlink.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> >
>
> You may want to write something about why this would be wanted.

Ok.

>
> > diff --git a/ell/netlink.c b/ell/netlink.c
> > index 33d0e5b..4664762 100644
> > --- a/ell/netlink.c
> > +++ b/ell/netlink.c
> > @@ -162,12 +162,15 @@ static void process_message(struct l_netlink *netlink, struct nlmsghdr *nlmsg)
> >   {
> >       const void *data = nlmsg;
> >       struct command *command;
> > +     uint32_t id;
> >
> >       command = l_hashmap_remove(netlink->command_pending,
> >                                       L_UINT_TO_PTR(nlmsg->nlmsg_seq));
> >       if (!command)
> >               return;
> >
> > +     id = command->id;
> > +
> >       if (!command->handler)
> >               goto done;
> >
> > @@ -189,9 +192,8 @@ static void process_message(struct l_netlink *netlink, struct nlmsghdr *nlmsg)
> >       }
> >
> >   done:
> > -     l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(command->id));
> > -
> > -     destroy_command(command);
> > +     if (l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(id)))
> > +             destroy_command(command);
>
> Would it not be better to simply remove the command from the command_lookup
> hashmap prior to dispatch, and have l_netlink_cancel simply fail?

Yes, that would work.

>
> I do wonder why you'd cancel the command from the handler?

In the IWD AP case I want to cancel a dump command which I think is
already handled correctly, but if the API can be made more robust then
I think it's an improvement.  There can be complicated code paths
where you destroy an object that owns the netlink command as a result
of the command response.

Best regards

Re: [PATCH] netlink: Allow command handler to use l_netlink_cancel

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

Hi Andrew,

>> I do wonder why you'd cancel the command from the handler?
> 
> In the IWD AP case I want to cancel a dump command which I think is
> already handled correctly, but if the API can be made more robust then
> I think it's an improvement.  There can be complicated code paths
> where you destroy an object that owns the netlink command as a result
> of the command response.

Sure, I buy the more robust argument, though I still wonder how one would even 
get into such a state on purpose.  But the last sentence seems to be handwaving? 
  So did you somehow trigger this condition or just fixing ghost bugs?

Regards,
-Denis

Re: [PATCH] netlink: Allow command handler to use l_netlink_cancel

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

On Tue, 2 Mar 2021 at 17:35, Denis Kenzior <denkenz@gmail.com> wrote:
> > In the IWD AP case I want to cancel a dump command which I think is
> > already handled correctly, but if the API can be made more robust then
> > I think it's an improvement.  There can be complicated code paths
> > where you destroy an object that owns the netlink command as a result
> > of the command response.
>
> Sure, I buy the more robust argument, though I still wonder how one would even
> get into such a state on purpose.  But the last sentence seems to be handwaving?

Well, in the AP code we're actually careful to reset e.g.
ap->rtnl_add_cmd to 0 at the beginning of the reply handler but if we
were resetting it any later we'd be affected by this.

>   So did you somehow trigger this condition or just fixing ghost bugs?

I was only checking the code to see if it was safe to cancel the dump
command the way I did it in my IWD patchset.  It seemed to be safe for
dump commands but not for regular commands.

The normal sequence is this:
1. l_netlink_send call
2. handler callback
3. destroy callback

After 3. I'd consider it wrong to call l_netlink_cancel() with the
original ID because you'd be risking cancelling an unrelated command
(there's no guarantee that the ID wasn't re-used since), but until 3.
I think it should be made safe.

Best regards

Re: [PATCH] netlink: Allow command handler to use l_netlink_cancel

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2473 bytes --]

Hi Andrew

On 3/2/21 3:30 PM, Andrew Zaborowski wrote:
> On Tue, 2 Mar 2021 at 17:35, Denis Kenzior <denkenz@gmail.com> wrote:
>>> In the IWD AP case I want to cancel a dump command which I think is
>>> already handled correctly, but if the API can be made more robust then
>>> I think it's an improvement.  There can be complicated code paths
>>> where you destroy an object that owns the netlink command as a result
>>> of the command response.
>>
>> Sure, I buy the more robust argument, though I still wonder how one would even
>> get into such a state on purpose.  But the last sentence seems to be handwaving?
> 
> Well, in the AP code we're actually careful to reset e.g.
> ap->rtnl_add_cmd to 0 at the beginning of the reply handler but if we
> were resetting it any later we'd be affected by this.
> 

Hmm... This is pretty typical pattern for when one doesn't provide a destroy 
callback.  I'm not sure I understand what you're trying to convey here...?

Are you trying to stop callbacks mid-dump or ?

>>    So did you somehow trigger this condition or just fixing ghost bugs?
> 
> I was only checking the code to see if it was safe to cancel the dump
> command the way I did it in my IWD patchset.  It seemed to be safe for
> dump commands but not for regular commands.

Yes, but *why* would you use l_netlink_cancel for the command when you're 
already in the handler for that exact same command?

> 
> The normal sequence is this:
> 1. l_netlink_send call
> 2. handler callback
> 3. destroy callback
> 
> After 3. I'd consider it wrong to call l_netlink_cancel() with the
> original ID because you'd be risking cancelling an unrelated command
> (there's no guarantee that the ID wasn't re-used since), but until 3.
> I think it should be made safe.

I can't really imagine why anyone would invoke l_netlink_cancel in a callback. 
Not saying we shouldn't allow this, but the use case is still a mystery to me?

Also, we sort of cheat in many places and assume that only a single message will 
be sent for certain commands.  Underneath, there's no real difference between a 
dump and a non-dump.  In fact the kernel might (someday) choose to send multiple 
messages as a reply.  The only way you know that you're done (for netlink/genl 
at least) is after the destroy callback has been called.  The callback itself 
has no way of knowing whether it is a single-part or multi-part message...

Regards,
-Denis

Re: [PATCH] netlink: Allow command handler to use l_netlink_cancel

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3578 bytes --]

On Wed, 3 Mar 2021 at 00:29, Denis Kenzior <denkenz@gmail.com> wrote:
> On 3/2/21 3:30 PM, Andrew Zaborowski wrote:
> > On Tue, 2 Mar 2021 at 17:35, Denis Kenzior <denkenz@gmail.com> wrote:
> >>> In the IWD AP case I want to cancel a dump command which I think is
> >>> already handled correctly, but if the API can be made more robust then
> >>> I think it's an improvement.  There can be complicated code paths
> >>> where you destroy an object that owns the netlink command as a result
> >>> of the command response.
> >>
> >> Sure, I buy the more robust argument, though I still wonder how one would even
> >> get into such a state on purpose.  But the last sentence seems to be handwaving?
> >
> > Well, in the AP code we're actually careful to reset e.g.
> > ap->rtnl_add_cmd to 0 at the beginning of the reply handler but if we
> > were resetting it any later we'd be affected by this.
> >
>
> Hmm... This is pretty typical pattern for when one doesn't provide a destroy
> callback.  I'm not sure I understand what you're trying to convey here...?
>
> Are you trying to stop callbacks mid-dump or ?

Yes, in the dump case the use case is pretty obvious -- once you've
received the fragment you were interested in (in my case, I dump IPv4
addresses and I'm interested in one specific ifindex), if you can
cancel the command you don't have to keep a flag that tells you to
ignore further callbacks.  And if you need to destroy the object (e.g.
stop the AP) based on the data you received, you're also free to do
this.

In the non-dump case I see the utility of this change mainly in the
situation where, within the callback, you decide you want to tear-down
the object (e.g. stop the AP), like I said already.  If you're not
allowed to cancel the command at that point, you basically need
reference counting so that you can avoid crashing in your destroy
handler.  And in that case I'd say we should have at least a warning
about this limitation somewhere.

>
> >>    So did you somehow trigger this condition or just fixing ghost bugs?
> >
> > I was only checking the code to see if it was safe to cancel the dump
> > command the way I did it in my IWD patchset.  It seemed to be safe for
> > dump commands but not for regular commands.
>
> Yes, but *why* would you use l_netlink_cancel for the command when you're
> already in the handler for that exact same command?
>
> >
> > The normal sequence is this:
> > 1. l_netlink_send call
> > 2. handler callback
> > 3. destroy callback
> >
> > After 3. I'd consider it wrong to call l_netlink_cancel() with the
> > original ID because you'd be risking cancelling an unrelated command
> > (there's no guarantee that the ID wasn't re-used since), but until 3.
> > I think it should be made safe.
>
> I can't really imagine why anyone would invoke l_netlink_cancel in a callback.
> Not saying we shouldn't allow this, but the use case is still a mystery to me?
>
> Also, we sort of cheat in many places and assume that only a single message will
> be sent for certain commands.  Underneath, there's no real difference between a
> dump and a non-dump.  In fact the kernel might (someday) choose to send multiple
> messages as a reply.  The only way you know that you're done (for netlink/genl
> at least) is after the destroy callback has been called.  The callback itself
> has no way of knowing whether it is a single-part or multi-part message...

Oh right, the dump/non-dump distinction in my example is based on the
current rtnl behavior.

Best regards

Re: [PATCH] netlink: Allow command handler to use l_netlink_cancel

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

Hi Andrew,

>> Are you trying to stop callbacks mid-dump or ?
> 
> Yes, in the dump case the use case is pretty obvious -- once you've
> received the fragment you were interested in (in my case, I dump IPv4
> addresses and I'm interested in one specific ifindex), if you can
> cancel the command you don't have to keep a flag that tells you to
> ignore further callbacks.  And if you need to destroy the object (e.g.
> stop the AP) based on the data you received, you're also free to do
> this.

Ok, gotcha.  I have to ponder whether using l_foo_cancel is a good idea for 
this, or a more explicit API should be used.  Also, we'd have to make this 
behavior consistent everywhere.

> 
> In the non-dump case I see the utility of this change mainly in the
> situation where, within the callback, you decide you want to tear-down
> the object (e.g. stop the AP), like I said already.  If you're not
> allowed to cancel the command at that point, you basically need
> reference counting so that you can avoid crashing in your destroy
> handler.  And in that case I'd say we should have at least a warning
> about this limitation somewhere.

You might be the first one to need this actually.  There's always the 
l_idle_oneshot get-out-of-jail card ;)

Regards,
-Denis

Re: [PATCH] netlink: Allow command handler to use l_netlink_cancel

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

Hi Denis,

On Tue, 2 Mar 2021 at 16:34, Andrew Zaborowski
<andrew.zaborowski@intel.com> wrote:
> On Tue, 2 Mar 2021 at 16:10, Denis Kenzior <denkenz@gmail.com> wrote:
> > >   done:
> > > -     l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(command->id));
> > > -
> > > -     destroy_command(command);
> > > +     if (l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(id)))
> > > +             destroy_command(command);
> >
> > Would it not be better to simply remove the command from the command_lookup
> > hashmap prior to dispatch, and have l_netlink_cancel simply fail?
>
> Yes, that would work.

On a second thought, I think it's better the other way.  The
l_netlink_cancel() call can be used as a request to forget the
callbacks.  The user may be tearing down the object they passed as
user_data for example.  IIRC we did take that use case into
consideration in some of the other APIs, what do you think?

>
> >
> > I do wonder why you'd cancel the command from the handler?
>
> In the IWD AP case I want to cancel a dump command which I think is
> already handled correctly, but if the API can be made more robust then
> I think it's an improvement.  There can be complicated code paths
> where you destroy an object that owns the netlink command as a result
> of the command response.
>
> Best regards

Re: [PATCH] netlink: Allow command handler to use l_netlink_cancel

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

Hi Andrew,

>>> Would it not be better to simply remove the command from the command_lookup
>>> hashmap prior to dispatch, and have l_netlink_cancel simply fail?
>>
>> Yes, that would work.
> 
> On a second thought, I think it's better the other way.  The
> l_netlink_cancel() call can be used as a request to forget the
> callbacks.  The user may be tearing down the object they passed as
> user_data for example.  IIRC we did take that use case into
> consideration in some of the other APIs, what do you think?

Which other way?  Ahhh, I think I get it... You want to unset the destroy 
callback or prevent it from being called subsequently when inside the handler by 
issuing l_netlink_cancel?

I actually don't think any ell APIs work this way.  l_dbus for example simply 
removes the pending request prior to dispatch (which is what I suggested 
earlier).  Hence l_dbus_cancel wouldn't work on it inside the l_dbus_send callback.

Even APIs that use the watchlist pattern don't do it this way.  You may be 
thinking of the scan API inside iwd, which did it for a different reason.

Regards,
-Denis