Re: [PATCH 4/5] staging: nvec: fix pm_power_off teardown in tegra_nvec_remove()

Re: [PATCH 4/5] staging: nvec: fix pm_power_off teardown in tegra_nvec_remove()

[Alexandru Hossu]

On Mon, Apr 13, 2026, Dan Carpenter wrote:
> At this point, we're unloading the driver so nvec_power_handle is
> about to be freed. Is there any benefit to setting it to NULL?

nvec_power_off() dereferences nvec_power_handle to send the power-off
command to the EC. If pm_power_off somehow gets reassigned to
nvec_power_off after our driver unloads (e.g. by a re-probe), the stale
nvec_power_handle would point to freed memory.

Setting it to NULL makes the potential failure mode explicit rather than
a silent use-after-free. Since we are already inside the if() guard,
the cost is a single pointer store.

Alexandru

Re: [PATCH 4/5] staging: nvec: fix pm_power_off teardown in tegra_nvec_remove()

[Dan Carpenter]

On Mon, Apr 13, 2026 at 03:02:50AM -0700, Alexandru Hossu wrote:
> On Mon, Apr 13, 2026, Dan Carpenter wrote:
> > At this point, we're unloading the driver so nvec_power_handle is
> > about to be freed. Is there any benefit to setting it to NULL?
> 
> nvec_power_off() dereferences nvec_power_handle to send the power-off
> command to the EC. If pm_power_off somehow gets reassigned to
> nvec_power_off after our driver unloads (e.g. by a re-probe), the stale
> nvec_power_handle would point to freed memory.

I like to believe it's impossible to reprobe a driver before the
rmmod has completed.  I'm not going to check on this, I'm just going
to take it on faith.  :P

> 
> Setting it to NULL makes the potential failure mode explicit rather than
> a silent use-after-free. Since we are already inside the if() guard,
> the cost is a single pointer store.

So the bug here is that we're racing an rmmod against a poweroff and we
trigger a bug.  And the fix is to change the use after free bug into a
NULL dereference.  Both of rmmod and poweroff are privileged operations
so you kind of get what you deserve if you do that.

I understand that it costs nothing to do the nvec_power_handle = NULL;
and if this were a new driver, I wouldn't comment on it.  (Although I
know other people who would).  But for a new patch, I'm just not sold
on this.  It makes the patch more confusing for no benefit.

regards,
dan carpenter

Re: [PATCH 4/5] staging: nvec: fix pm_power_off teardown in tegra_nvec_remove()

[Dan Carpenter]

I haven't looked at if you can actually power off a system while an rmmod
is in progress...  I feel like a real old grandpa when I say things like,
"Back in my day you weren't supposed to call rmmod on a production
system, the rmmod feature was only for debugging."

But the bigger picture is that linux power off is a mess.  There are a
dozen different ways to power off a system.  Here we have a driver
which just takes over the power off process by assigning a function
pointer.  Shouldn't we instead have a list of ways to shut down the
system and have a standard way to pick which is the correct one?

Instead of worrying too much about this one driver it would be better
focus on the larger picture.

regards,
dan carpenter

Re: [PATCH 4/5] staging: nvec: fix unconditional pm_power_off teardown

[Alexandru Hossu]

On Mon, Apr 13, 2026, Dan Carpenter wrote:
> Can you resend as a new thread?  Mixing nvec and ipu7 in one series is
> a bit confusing.
>
> Also you can drop the "nvec_power_handle = NULL;" line.  After we set
> pm_power_off = NULL then nvec_power_off() is no longer reachable so
> there is no real bug.

Fair point, thanks. Resent as a clean [PATCH v2 1/2] + [PATCH v2 2/2]
series covering only the nvec fixes. The nvec_power_handle = NULL line
is dropped in v2.

Alexandru