[PATCH net v1] net: phy: fix NULL deref in IRQ handler after unbind

[PATCH net v1] net: phy: fix NULL deref in IRQ handler after unbind

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

phydev->drv can become NULL while the phy_device is still attached to
its net_device, after the PHY driver is unbound via sysfs:

    echo <mdio_id> > /sys/bus/mdio_bus/drivers/<phy_drv>/unbind

phy_remove() clears phydev->drv but does not free the threaded IRQ
installed at attach time. A later hardware interrupt, or another
device on the same IRQF_SHARED line, then oopses in phy_interrupt()
on phydev->drv->handle_interrupt():

    Unable to handle kernel NULL pointer dereference at 0x138
    ...
    pc : phy_interrupt+0xb4/0x108
    Call trace:
     phy_interrupt+0xb4/0x108 (P)
     irq_thread_fn+0x34/0xb8
     irq_thread+0xc8/0x178
     kthread+0x128/0x138
     ret_from_fork+0x10/0x20

The same unbind sequence is already treated as valid. Skip the
driver callback when no PHY driver is bound, matching other phylib
unbind paths. Also guard phy_config_interrupt(), which is reached
from phy_disconnect() after the same unbind.

The IRQ is requested at attach time and released by phy_disconnect(),
so phy_remove() cannot free it without a later double-free. After
this change the handler returns IRQ_NONE. If the PHY keeps the line
asserted and no other shared handler claims it, the IRQ core may
eventually disable the whole line ("nobody cared") and affect other
devices on that IRQ. That is a limitation of not tearing the IRQ
down on driver unbind, but it is still better than the NULL pointer
dereference that the same interrupt triggers today.

Fixes: c974bdbc3e77 ("net: phy: Use threaded IRQ, to allow IRQ from sleeping devices")
Cc: stable@vger.kernel.org
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/phy.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index fce9bc7be3304..5435f62e98171 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -168,7 +168,7 @@ EXPORT_SYMBOL_GPL(phy_get_rate_matching);
 static int phy_config_interrupt(struct phy_device *phydev, bool interrupts)
 {
 	phydev->interrupts = interrupts ? 1 : 0;
-	if (phydev->drv->config_intr)
+	if (phydev->drv && phydev->drv->config_intr)
 		return phydev->drv->config_intr(phydev);
 
 	return 0;
@@ -1436,7 +1436,10 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	}
 
 	mutex_lock(&phydev->lock);
-	ret = phydev->drv->handle_interrupt(phydev);
+	if (!phydev->drv)
+		ret = IRQ_NONE;
+	else
+		ret = phydev->drv->handle_interrupt(phydev);
 	mutex_unlock(&phydev->lock);
 
 	return ret;

Re: [PATCH net v1] net: phy: fix NULL deref in IRQ handler after unbind

[Andrew Lunn]

> The IRQ is requested at attach time and released by phy_disconnect(),
> so phy_remove() cannot free it without a later double-free.

So this sounds wrong.

If you unbind the PHY, you need to also unbind the MAC, since a MAC
without a PHY is useless. When the MAC unloads, it will call
phy_remove() so everything unwinds in the correct order.

    Andrew

---
pw-bot: cr
	     

Re: [PATCH net v1] net: phy: fix NULL deref in IRQ handler after unbind

[Xuanqiang Luo]

Hi Andrew,

在 2026/8/24 20:54, Andrew Lunn 写道:

>> The IRQ is requested at attach time and released by phy_disconnect(),
>> so phy_remove() cannot free it without a later double-free.
> So this sounds wrong.
>
> If you unbind the PHY, you need to also unbind the MAC, since a MAC
> without a PHY is useless. When the MAC unloads, it will call
> phy_remove() so everything unwinds in the correct order.
>
>      Andrew
>
> ---
> pw-bot: cr
> 	

I agree that the MAC should normally be unbound before the PHY.

Also, the paragraph about freeing the IRQ in phy_remove() was
misleading. It was not relevant to the change being proposed,
so I will drop it in v2.

Do you mean that unbinding the PHY first through sysfs is not a
supported operation?

The same sequence was used to reproduce the issue fixed by commit
c2b727df7caa ("net: phy: Avoid NPD upon phy_detach() when driver is
unbound"), which made me think that it should at least not crash:

https://lore.kernel.org/all/20200917034310.2360488-2-f.fainelli@gmail.com/

If this ordering is required, would the right fix be to enforce it
instead?

Thanks,
Xuanqiang

Re: [PATCH net v1] net: phy: fix NULL deref in IRQ handler after unbind

[Andrew Lunn]

On Tue, Aug 25, 2026 at 11:34:04AM +0800, Xuanqiang Luo wrote:
> Hi Andrew,
> 
> 在 2026/8/24 20:54, Andrew Lunn 写道:
> 
> > > The IRQ is requested at attach time and released by phy_disconnect(),
> > > so phy_remove() cannot free it without a later double-free.
> > So this sounds wrong.
> > 
> > If you unbind the PHY, you need to also unbind the MAC, since a MAC
> > without a PHY is useless. When the MAC unloads, it will call
> > phy_remove() so everything unwinds in the correct order.
> > 
> >      Andrew
> > 
> > ---
> > pw-bot: cr
> > 	
> 
> I agree that the MAC should normally be unbound before the PHY.
> 
> Also, the paragraph about freeing the IRQ in phy_remove() was
> misleading. It was not relevant to the change being proposed,
> so I will drop it in v2.
> 
> Do you mean that unbinding the PHY first through sysfs is not a
> supported operation?

To me, unbind is an odd thing to do. What is your use case?

When doing development work, i tend to reboot the target. If not, i
would unload the MAC driver and the PHY driver, to ensure i have a
clean state.

> 
> The same sequence was used to reproduce the issue fixed by commit
> c2b727df7caa ("net: phy: Avoid NPD upon phy_detach() when driver is
> unbound"), which made me think that it should at least not crash:
> 
> https://lore.kernel.org/all/20200917034310.2360488-2-f.fainelli@gmail.com/

A crash is not good.

But we also need to look, is the fix the correct architecturally, or
we are actually making it worse. Drivers have function pairs. A
function which does setup, and a mirror function which does
tairdown. A function to bind to a MAC and a mirror which unbinds from
a MAC. That symmetry makes drivers simple, easy to reason about.

Does this problem happen if you keep to the symmetry?

Does your fix to this problem make the symmetry worse?

If unbind were to cause the MAC to unload, is the symmetry kept?

   Andrew

Re: [PATCH net v1] net: phy: fix NULL deref in IRQ handler after unbind

[Xuanqiang Luo]

Hi Andrew,

在 2026/8/25 21:03, Andrew Lunn 写道:

> On Tue, Aug 25, 2026 at 11:34:04AM +0800, Xuanqiang Luo wrote:
>> Hi Andrew,
>>
>> 在 2026/8/24 20:54, Andrew Lunn 写道:
>>
>>>> The IRQ is requested at attach time and released by phy_disconnect(),
>>>> so phy_remove() cannot free it without a later double-free.
>>> So this sounds wrong.
>>>
>>> If you unbind the PHY, you need to also unbind the MAC, since a MAC
>>> without a PHY is useless. When the MAC unloads, it will call
>>> phy_remove() so everything unwinds in the correct order.
>>>
>>>       Andrew
>>>
>>> ---
>>> pw-bot: cr
>>> 	
>> I agree that the MAC should normally be unbound before the PHY.
>>
>> Also, the paragraph about freeing the IRQ in phy_remove() was
>> misleading. It was not relevant to the change being proposed,
>> so I will drop it in v2.
>>
>> Do you mean that unbinding the PHY first through sysfs is not a
>> supported operation?
> To me, unbind is an odd thing to do. What is your use case?

I don't have a real use case for unbinding the PHY while keeping the
MAC bound. I found this during code review and used sysfs unbind to
reproduce the crash.

> When doing development work, i tend to reboot the target. If not, i
> would unload the MAC driver and the PHY driver, to ensure i have a
> clean state.
>
>> The same sequence was used to reproduce the issue fixed by commit
>> c2b727df7caa ("net: phy: Avoid NPD upon phy_detach() when driver is
>> unbound"), which made me think that it should at least not crash:
>>
>> https://lore.kernel.org/all/20200917034310.2360488-2-f.fainelli@gmail.com/
> A crash is not good.
>
> But we also need to look, is the fix the correct architecturally, or
> we are actually making it worse. Drivers have function pairs. A
> function which does setup, and a mirror function which does
> tairdown. A function to bind to a MAC and a mirror which unbinds from
> a MAC. That symmetry makes drivers simple, easy to reason about.

Thanks for clarifying. I agree.

> Does this problem happen if you keep to the symmetry?

No. I checked the normal teardown path, and the problem does not occur
when the symmetric teardown order is followed. This is expected.

> Does your fix to this problem make the symmetry worse?

The normal teardown path is unchanged, but the patch allows the
asymmetric state created by a PHY-first unbind to continue with the IRQ
still installed. From that point of view, the fix is wrong.

> If unbind were to cause the MAC to unload, is the symmetry kept?

Yes. I agree that this is the right direction. I will stop here and
think more carefully about how to enforce the ordering before posting
another version.

Thanks,
Xuanqiang