[PATCH net v2] atm: Fix the cleanup on alloc_mpc failure in atm_mpoa_mpoad_attach

[PATCH net v2] atm: Fix the cleanup on alloc_mpc failure in atm_mpoa_mpoad_attach

[Deepak Sharma]

Syzbot reported a warning at `add_timer`, which is called from the
`atm_mpoa_mpoad_attach` function

The reason for warning is that in the first call to the ioctl, if
there is no MPOA client created yet (mpcs is the linked list for
these MPOA clients) we do a `mpc_timer_refresh` to arm the timer.
Later on, if the `alloc_mpc` fails (which on success will also
initialize mpcs if it's first MPOA client created) and we didn't
have any MPOA client yet, we return without the timer de-armed

If the same ioctl is called again, since we don't have any MPOA
clients yet we again arm the timer, which might already be left
armed by the previous call to this ioctl in which `alloc_mpc` failed

Hence, de-arm the timer in the event that `alloc_mpc` fails and we
don't have any other MPOA client (that is, `mpcs` is NULL)

Do a `timer_delete_sync` instead of `timer_delete`, since the timer
callback can arm it back again

This does not need to be done at the early return in case of
`mpc->mpoad_vcc`, or a control channel to MPOAD already exists.
The timer should remain there to periodically process caches

Reported-by: syzbot+07b635b9c111c566af8b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=07b635b9c111c566af8b
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Deepak Sharma <deepak.sharma.472935@gmail.com>
---
v2:
 - Improved commit message
 - Fix the faulty condition check to disarm the timer
 - Use `timer_delete_sync` instead to avoid re-arming of timer

v1:
 - Disarm the timer using `timer_delete` in case `alloc_mpc`
   fails`

 net/atm/mpc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/atm/mpc.c b/net/atm/mpc.c
index f6b447bba329..4f67ad1d6bef 100644
--- a/net/atm/mpc.c
+++ b/net/atm/mpc.c
@@ -804,7 +804,7 @@ static int atm_mpoa_mpoad_attach(struct atm_vcc *vcc, int arg)
 		/* This lets us now how our LECs are doing */
 		err = register_netdevice_notifier(&mpoa_notifier);
 		if (err < 0) {
-			timer_delete(&mpc_timer);
+			timer_delete_sync(&mpc_timer);
 			return err;
 		}
 	}
@@ -813,8 +813,10 @@ static int atm_mpoa_mpoad_attach(struct atm_vcc *vcc, int arg)
 	if (mpc == NULL) {
 		dprintk("allocating new mpc for itf %d\n", arg);
 		mpc = alloc_mpc();
-		if (mpc == NULL)
+		if (!mpcs) {
+			timer_delete_sync(&mpc_timer);
 			return -ENOMEM;
+		}
 		mpc->dev_num = arg;
 		mpc->dev = find_lec_by_itfnum(arg);
 					/* NULL if there was no lec */
-- 
2.51.0

Re: [PATCH net v2] atm: Fix the cleanup on alloc_mpc failure in atm_mpoa_mpoad_attach

[Simon Horman]

On Fri, Sep 26, 2025 at 02:12:51AM +0530, Deepak Sharma wrote:
> Syzbot reported a warning at `add_timer`, which is called from the
> `atm_mpoa_mpoad_attach` function
> 
> The reason for warning is that in the first call to the ioctl, if
> there is no MPOA client created yet (mpcs is the linked list for
> these MPOA clients) we do a `mpc_timer_refresh` to arm the timer.
> Later on, if the `alloc_mpc` fails (which on success will also
> initialize mpcs if it's first MPOA client created) and we didn't
> have any MPOA client yet, we return without the timer de-armed
> 
> If the same ioctl is called again, since we don't have any MPOA
> clients yet we again arm the timer, which might already be left
> armed by the previous call to this ioctl in which `alloc_mpc` failed
> 
> Hence, de-arm the timer in the event that `alloc_mpc` fails and we
> don't have any other MPOA client (that is, `mpcs` is NULL)
> 
> Do a `timer_delete_sync` instead of `timer_delete`, since the timer
> callback can arm it back again
> 
> This does not need to be done at the early return in case of
> `mpc->mpoad_vcc`, or a control channel to MPOAD already exists.
> The timer should remain there to periodically process caches
> 
> Reported-by: syzbot+07b635b9c111c566af8b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=07b635b9c111c566af8b
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Deepak Sharma <deepak.sharma.472935@gmail.com>
> ---
> v2:
>  - Improved commit message
>  - Fix the faulty condition check to disarm the timer
>  - Use `timer_delete_sync` instead to avoid re-arming of timer
> 
> v1:
>  - Disarm the timer using `timer_delete` in case `alloc_mpc`
>    fails`

Thanks for the update.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net v2] atm: Fix the cleanup on alloc_mpc failure in atm_mpoa_mpoad_attach

[Paolo Abeni]

On 9/25/25 10:42 PM, Deepak Sharma wrote:
> diff --git a/net/atm/mpc.c b/net/atm/mpc.c
> index f6b447bba329..4f67ad1d6bef 100644
> --- a/net/atm/mpc.c
> +++ b/net/atm/mpc.c
> @@ -804,7 +804,7 @@ static int atm_mpoa_mpoad_attach(struct atm_vcc *vcc, int arg)
>  		/* This lets us now how our LECs are doing */
>  		err = register_netdevice_notifier(&mpoa_notifier);
>  		if (err < 0) {
> -			timer_delete(&mpc_timer);
> +			timer_delete_sync(&mpc_timer);

AFAICS the mpc_timer can rearm itself, so this the above is not enough
and you should use timer_shutdown_sync() instead.

Thanks,

Paolo

Re: [PATCH net v2] atm: Fix the cleanup on alloc_mpc failure in atm_mpoa_mpoad_attach

[Deepak Sharma]

On Tue, Sep 30, 2025 at 2:15 PM Paolo Abeni <pabeni@redhat.com> wrote:
> AFAICS the mpc_timer can rearm itself, so this the above is not enough
> and you should use timer_shutdown_sync() instead.

Hi,

As I understand it, `timer_shutdown_sync` will prevent any further
re-arming of the timer. I think this is not what we want here; since even if
we somehow fail to allocate our first MPOA client object on our first
ioctl call,
and hence end up wanting to disarm the timer, maybe on next call we can
allocate it successfully, and we would want that caches are processed
(which are processed for every time out). So we still want it to be
possible that
we can re-arm it.

And as I understand, `timer_delete_sync` will wait for the callback to
finish, so
deleting the timer after that should do the job

Thanks,

Deepak

Re: [PATCH net v2] atm: Fix the cleanup on alloc_mpc failure in atm_mpoa_mpoad_attach

[Paolo Abeni]

On 9/30/25 3:33 PM, Deepak Sharma wrote:
> On Tue, Sep 30, 2025 at 2:15 PM Paolo Abeni <pabeni@redhat.com> wrote:
>> AFAICS the mpc_timer can rearm itself, so this the above is not enough
>> and you should use timer_shutdown_sync() instead.
> 
> Hi,
> 
> As I understand it, `timer_shutdown_sync` will prevent any further
> re-arming of the timer. I think this is not what we want here; since even if
> we somehow fail to allocate our first MPOA client object on our first
> ioctl call,
> and hence end up wanting to disarm the timer, maybe on next call we can
> allocate it successfully, and we would want that caches are processed
> (which are processed for every time out). So we still want it to be
> possible that
> we can re-arm it.

Ah, I missed the goal here is just being able to rearm the timer (i.e.
there is no related UaF).

Given the above, I think you could instead simply replace add_timer()
with mod_timer().

/P

Re: [PATCH net v2] atm: Fix the cleanup on alloc_mpc failure in atm_mpoa_mpoad_attach

[Cortex Auth]

On Tue, Sep 30, 2025 at 8:01 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 9/30/25 3:33 PM, Deepak Sharma wrote:
> > On Tue, Sep 30, 2025 at 2:15 PM Paolo Abeni <pabeni@redhat.com> wrote:
> >> AFAICS the mpc_timer can rearm itself, so this the above is not enough
> >> and you should use timer_shutdown_sync() instead.
> >
> > Hi,
> >
> > As I understand it, `timer_shutdown_sync` will prevent any further
> > re-arming of the timer. I think this is not what we want here; since even if
> > we somehow fail to allocate our first MPOA client object on our first
> > ioctl call,
> > and hence end up wanting to disarm the timer, maybe on next call we can
> > allocate it successfully, and we would want that caches are processed
> > (which are processed for every time out). So we still want it to be
> > possible that
> > we can re-arm it.
>
> Ah, I missed the goal here is just being able to rearm the timer (i.e.
> there is no related UaF).
>
> Given the above, I think you could instead simply replace add_timer()
> with mod_timer().
>
> /P
>

I think yeah we could do that.

I have just been going with what code seems to have wanted to do;
Arm the timer if `mpcs` was NULL (no MPOA client existed)
And if there's any error, delete it as (was done in case of error by
the `_notifier` call, where we have no MPOA client yet).

I just extended it to the `alloc_mpoa` failure too, because
in that case too `mpcs` remains NULL

`mod_timer` would still work, because the timer callback will not do much
if it finds the `mpcs` to be NULL

If it sounds good, I can go ahead with it

Thanks,

Deepak