[PATCH net] net/ncsi: Don't assume last available channel exists

[PATCH net] net/ncsi: Don't assume last available channel exists

[Samuel Mendoza-Jonas]

When handling new VLAN tags in NCSI we check the maximum allowed number
of filters on the last active ("hot") channel. However if the 'add'
callback is called before NCSI has configured a channel, this causes a
NULL dereference.

Check that we actually have a hot channel, and warn if it is missing.

Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
---
 net/ncsi/ncsi-manage.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 3fd3c39e6278..fc800f934beb 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -1420,7 +1420,10 @@ int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
 	}
 
 	ndp = TO_NCSI_DEV_PRIV(nd);
-	ncf = ndp->hot_channel->filters[NCSI_FILTER_VLAN];
+	if (!ndp) {
+		netdev_warn(dev, "ncsi: No ncsi_dev_priv?\n");
+		return 0;
+	}
 
 	/* Add the VLAN id to our internal list */
 	list_for_each_entry_rcu(vlan, &ndp->vlan_vids, list) {
@@ -1432,11 +1435,17 @@ int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
 		}
 	}
 
-	if (n_vids >= ncf->total) {
-		netdev_info(dev,
-			    "NCSI Channel supports up to %u VLAN tags but %u are already set\n",
-			    ncf->total, n_vids);
-		return -EINVAL;
+	if (!ndp->hot_channel) {
+		netdev_warn(dev,
+			    "ncsi: no available filter to check maximum\n");
+	} else {
+		ncf = ndp->hot_channel->filters[NCSI_FILTER_VLAN];
+		if (n_vids >= ncf->total) {
+			netdev_info(dev,
+				    "NCSI Channel supports up to %u VLAN tags but %u are already set\n",
+				    ncf->total, n_vids);
+			return -EINVAL;
+		}
 	}
 
 	vlan = kzalloc(sizeof(*vlan), GFP_KERNEL);
-- 
2.14.1

Re: [PATCH net] net/ncsi: Don't assume last available channel exists

[David Miller]

From: Samuel Mendoza-Jonas <sam@mendozajonas.com>
Date: Wed, 20 Sep 2017 14:12:51 +1000

> When handling new VLAN tags in NCSI we check the maximum allowed number
> of filters on the last active ("hot") channel. However if the 'add'
> callback is called before NCSI has configured a channel, this causes a
> NULL dereference.
> 
> Check that we actually have a hot channel, and warn if it is missing.
> 
> Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>

Well, a few things.

We should not allow this driver method to be invoked in the first
place if the device is not in a state where the operation is legal
yet.

Second of all, if !ndp is true, you should not return 0 to indicate
success.

But more fundamentally, we should block this method from being
callable unless the device is in the proper state and can complete the
operation.

Seriously, these checks should be completely unnecessary if those
issues are handled properly.

Re: [PATCH net] net/ncsi: Don't assume last available channel exists

[Samuel Mendoza-Jonas]

On Wed, 2017-09-20 at 16:05 -0700, David Miller wrote:
> From: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> Date: Wed, 20 Sep 2017 14:12:51 +1000
> 
> > When handling new VLAN tags in NCSI we check the maximum allowed number
> > of filters on the last active ("hot") channel. However if the 'add'
> > callback is called before NCSI has configured a channel, this causes a
> > NULL dereference.
> > 
> > Check that we actually have a hot channel, and warn if it is missing.
> > 
> > Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> 
> Well, a few things.
> 
> We should not allow this driver method to be invoked in the first
> place if the device is not in a state where the operation is legal
> yet.
> 
> Second of all, if !ndp is true, you should not return 0 to indicate
> success.
> 
> But more fundamentally, we should block this method from being
> callable unless the device is in the proper state and can complete the
> operation.
> 
> Seriously, these checks should be completely unnecessary if those
> issues are handled properly.

Good point, this made me step back and reconsider the problem here.

The ncsi_vlan_rx_add_vid() callback exists because the NCSI driver needs
to know which VLAN IDs are set, but we don't have a straightforward way
of accessing that information later in ncsi_configure_channel() - as
opposed to the MAC address for example which is just accessed via
dev->dev_addr. Instead they're kept track of in the ndp->vlan_vids list
and the NCSI driver reconfigures any channels it knows about.

So in that sense the NCSI device *is* ready to handle the operation. The
hot_channel fix here was to check if we were exceeding the maximum number
of VLAN filters supported by the remote channel. That itself is bit
debatable since different channels could support different numbers of
filters but right now the NCSI driver only supports one active channel at
a time.

If we haven't configured a channel yet (or are in the process of doing
so) we won't have a hot_channel - does it make more sense to
- check against the hot_channel as currently done,
- only check the filter size at configure time for /each/ channel,
- only conditionally enable the .ndo_vlan_rx_add_vid net_device callback
once we've configured a channel (eg. for ftgmac100 in the
ftgmac100_ncsi_handler() callback?)

Thanks,
Sam

Re: [PATCH net] net/ncsi: Don't assume last available channel exists

[David Miller]

From: Samuel Mendoza-Jonas <sam@mendozajonas.com>
Date: Fri, 22 Sep 2017 11:00:00 +1000

> If we haven't configured a channel yet (or are in the process of doing
> so) we won't have a hot_channel - does it make more sense to
> - check against the hot_channel as currently done,
> - only check the filter size at configure time for /each/ channel,
> - only conditionally enable the .ndo_vlan_rx_add_vid net_device callback
> once we've configured a channel (eg. for ftgmac100 in the
> ftgmac100_ncsi_handler() callback?)

The last isn't so feasible.

The device shouldn't be marked attached until a channel is available,
because it seems like communication cannot occur until one is.  Right?

You could experiment with netif_device_detach()/netif_device_attach().

When the device is in the detached state, callbacks such as
->ndo_vlan_rx_add_vid() will not be invoked.

Re: [PATCH net] net/ncsi: Don't assume last available channel exists

[Samuel Mendoza-Jonas]

On Thu, 2017-09-21 at 18:11 -0700, David Miller wrote:
> From: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> Date: Fri, 22 Sep 2017 11:00:00 +1000
> 
> > If we haven't configured a channel yet (or are in the process of doing
> > so) we won't have a hot_channel - does it make more sense to
> > - check against the hot_channel as currently done,
> > - only check the filter size at configure time for /each/ channel,
> > - only conditionally enable the .ndo_vlan_rx_add_vid net_device callback
> > once we've configured a channel (eg. for ftgmac100 in the
> > ftgmac100_ncsi_handler() callback?)
> 
> The last isn't so feasible.
> 
> The device shouldn't be marked attached until a channel is available,
> because it seems like communication cannot occur until one is.  Right?

Yes that's right.

> 
> You could experiment with netif_device_detach()/netif_device_attach().
> 
> When the device is in the detached state, callbacks such as
> ->ndo_vlan_rx_add_vid() will not be invoked.

This looked like the way at first, but _detach() ceases any tx/rx on the
interface right?
NCSI still needs the interface to be active since the 'channels' are on a
separate network controller that the interface is connected to, eg on the
machines I'm using:

BMC					     'Host' network controller
----------------------			     ----------------------------
|ftgmac100 interface | <---- NCSI Link ----> | BCM5719 interface	| --> external interface
----------------------			     ----------------------------

Looking at the NCSI init path I believe we're guaranteed to have an ndp
struct by the time ndo_vlan_rx_add_vid() is called, making some of those
checks overly cautious. It might be easiest to just track new vids as we
see them (up to the NCSI spec limit), and then deal with configured
channels on a case by case basis since their limits can be different.
I'll work on a V2 but hopefully I haven't misinterpreted
_detach()/_attach() :)

Sam