| Message ID | 20170920041251.14635-1-sam@mendozajonas.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net/ncsi: Don't assume last available channel exists | expand |
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.
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
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.
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
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);
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(-)