[PATCH net-next v1] net: core: fix logical inconsistency in failover_slave_register()

[PATCH net-next v1] net: core: fix logical inconsistency in failover_slave_register()

[Zeeshan Ahmad]

Smatch warns that 'fops' is dereferenced at line 66 before being checked
for NULL at line 85.

The current code inconsistently assumes 'fops' might be NULL at lines 62
and 85, while dereferencing it without a check at line 66.

Move the NULL check to the beginning of the function immediately after
'fops' is initialized. This ensures safety for all subsequent
dereferences and allows the removal of redundant checks.

Signed-off-by: Zeeshan Ahmad <zeeshanahmad022019@gmail.com>
---
 net/core/failover.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/core/failover.c b/net/core/failover.c
index 2a140b3ea669..88e376be0255 100644
--- a/net/core/failover.c
+++ b/net/core/failover.c
@@ -56,10 +56,10 @@ static int failover_slave_register(struct net_device *slave_dev)
 	ASSERT_RTNL();
 
 	failover_dev = failover_get_bymac(slave_dev->perm_addr, &fops);
-	if (!failover_dev)
+	if (!failover_dev || !fops)
 		goto done;
 
-	if (fops && fops->slave_pre_register &&
+	if (fops->slave_pre_register &&
 	    fops->slave_pre_register(slave_dev, failover_dev))
 		goto done;
 
@@ -82,7 +82,7 @@ static int failover_slave_register(struct net_device *slave_dev)
 
 	slave_dev->priv_flags |= (IFF_FAILOVER_SLAVE | IFF_NO_ADDRCONF);
 
-	if (fops && fops->slave_register &&
+	if (fops->slave_register &&
 	    !fops->slave_register(slave_dev, failover_dev))
 		return NOTIFY_OK;
 
-- 
2.43.0

Re: [PATCH net-next v1] net: core: fix logical inconsistency in failover_slave_register()

[Simon Horman]

On Thu, Feb 19, 2026 at 02:02:16PM +0500, Zeeshan Ahmad wrote:
> Smatch warns that 'fops' is dereferenced at line 66 before being checked
> for NULL at line 85.
> 
> The current code inconsistently assumes 'fops' might be NULL at lines 62
> and 85, while dereferencing it without a check at line 66.
> 
> Move the NULL check to the beginning of the function immediately after
> 'fops' is initialized. This ensures safety for all subsequent
> dereferences and allows the removal of redundant checks.
> 
> Signed-off-by: Zeeshan Ahmad <zeeshanahmad022019@gmail.com>

Hi Zeeshan,

It's not entirely clear to me what the behaviour should be if fops is
NULL, or indeed if fops can be NULL.

But looking over both this function and other users of failover_get_bymac()
it seems to me that the general patter is to skip steps that needs
to dereference fops if fops is NULL.

So I think it would be best to do the same here - that is modify the
code around line 66 to make it conditional on fops not being NULL.
Otherwise, if fops is NULL then steps that would have been taken are skipped.

It is true that in those steps would never be reached and the kernel would
have panic'ed due to a NULL dereference on line 66. So maybe your
approach is better, perhaps with the addition of a WARN_ON_ONCE.
But I feel that opens a larger surface that needs to be fixed in terms
of understanding other users of failover_get_bymac() and if they (still)
need to take fops being NULL into account.

LMKWYT


On process, if this is a fix - which I think means there is at least
a theoretical case where it can go *boom* - then please target at net
rather than net-next, and include a Fixes tag.

Else,

## Form letter - net-next-closed

We have already submitted our pull request with net-next material for v7.0,
and therefore net-next is closed for new drivers, features, code refactoring
and optimizations. We are currently accepting bug fixes only.

Please repost when net-next reopens after Feb 23rd.

RFC patches sent for review only are obviously welcome at any time.
                                                                                See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle
--
pw-bot: defer

Re: [PATCH net-next v1] net: core: fix logical inconsistency in failover_slave_register()

[Zeeshan Ahmad]

Hi Simon,

Thank you for the detailed feedback.

On Thu, Feb 19, 2026 at 2:40 PM Simon Horman <horms@kernel.org> wrote:
> It's not entirely clear to me what the behaviour should be if fops is
> NULL, or indeed if fops can be NULL.

I've performed a deeper audit of the failover module and found that
failover_register() currently allows a master instance to be registered
with ops = NULL. This appears to be the root of the issue. However, I
checked all current in-tree callers (e.g. net_failover.c) and confirmed
they always pass valid ops. So while it practically doesn't happen
today, the framework technically allows this inconsistent state.

> So I think it would be best to do the same here - that is modify the
> code around line 66 to make it conditional on fops not being NULL.
> Otherwise, if fops is NULL then steps that would have been taken are
> skipped.

Wouldn't skipping the rx_handler registration at line 66 lead to an
inconsistent state? If we skip that hook but continue to link the slave
to the master (line 75) and set the failover flags (line 83), the device
might appear linked but the data path would remain unhooked. This
concern is why I am leaning toward a more definitive "abort" if fops
is missing.

> It is true that in those steps would never be reached and the kernel
> would have panic'ed due to a NULL dereference on line 66. So maybe your
> approach is better, perhaps with the addition of a WARN_ON_ONCE.

I agree that WARN_ON_ONCE(!fops) is the best way to handle this. It
provides a clear signal to developers of a misconfiguration without
allowing the kernel to panic.

Based on this, I will prepare a v2 targeting the 'net' tree. It will use
the WARN_ON_ONCE check to both prevent the panic and abort the
registration (returning NOTIFY_DONE) to prevent an inconsistent failover
state.

Best regards,
Zeeshan

Re: [PATCH net-next v1] net: core: fix logical inconsistency in failover_slave_register()

[Simon Horman]

On Wed, Feb 25, 2026 at 03:04:25PM +0500, Zeeshan Ahmad wrote:
> Hi Simon,
> 
> Thank you for the detailed feedback.
> 
> On Thu, Feb 19, 2026 at 2:40 PM Simon Horman <horms@kernel.org> wrote:
> > It's not entirely clear to me what the behaviour should be if fops is
> > NULL, or indeed if fops can be NULL.
> 
> I've performed a deeper audit of the failover module and found that
> failover_register() currently allows a master instance to be registered
> with ops = NULL. This appears to be the root of the issue. However, I
> checked all current in-tree callers (e.g. net_failover.c) and confirmed
> they always pass valid ops. So while it practically doesn't happen
> today, the framework technically allows this inconsistent state.

Thanks. I did a not very deep audit before writing my previous email.
And my conclusion from that was the same as yours.

> 
> > So I think it would be best to do the same here - that is modify the
> > code around line 66 to make it conditional on fops not being NULL.
> > Otherwise, if fops is NULL then steps that would have been taken are
> > skipped.
> 
> Wouldn't skipping the rx_handler registration at line 66 lead to an
> inconsistent state? If we skip that hook but continue to link the slave
> to the master (line 75) and set the failover flags (line 83), the device
> might appear linked but the data path would remain unhooked. This
> concern is why I am leaning toward a more definitive "abort" if fops
> is missing.
> 
> > It is true that in those steps would never be reached and the kernel
> > would have panic'ed due to a NULL dereference on line 66. So maybe your
> > approach is better, perhaps with the addition of a WARN_ON_ONCE.
> 
> I agree that WARN_ON_ONCE(!fops) is the best way to handle this. It
> provides a clear signal to developers of a misconfiguration without
> allowing the kernel to panic.

Agree, that seems to be the best plan.

> Based on this, I will prepare a v2 targeting the 'net' tree. It will use
> the WARN_ON_ONCE check to both prevent the panic and abort the
> registration (returning NOTIFY_DONE) to prevent an inconsistent failover
> state.

Great, thanks.