Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1735432 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH net] net/ncsi: Don't assume last available channel exists Samuel Mendoza-Jonas <sam@mendozajonas.com> - 2017-09-20 06:20 +0200
    Re: [PATCH net] net/ncsi: Don't assume last available channel  exists David Miller <davem@davemloft.net> - 2017-09-21 01:10 +0200
      Re: [PATCH net] net/ncsi: Don't assume last available channel exists Samuel Mendoza-Jonas <sam@mendozajonas.com> - 2017-09-22 03:10 +0200
        Re: [PATCH net] net/ncsi: Don't assume last available channel  exists David Miller <davem@davemloft.net> - 2017-09-22 03:20 +0200

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

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

[toc] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

#1737118

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web