| Message ID | 20170108050157.16302-2-f.fainelli@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, Jan 07, 2017 at 09:01:56PM -0800, Florian Fainelli wrote: > We make the bcm_sf2 driver override ds->ops which points to > b53_switch_ops since b53_switch_alloc() did the assignent. This is all > well and good until a second b53 switch comes in, and ends up using the > bcm_sf2 operations. Make a proper local copy, substitute the ds->ops > pointer and then override the operations. > > Fixes: f458995b9ad8 ("net: dsa: bcm_sf2: Utilize core B53 driver when possible") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Hi Florian There is a general trend of making ops structures const. It closes off kernel exploits. This coping and then modifying prevents us making ds->ops a pointer to a const. You are already using b53_common.c as a library. Could you go further with the concept, and export the ops you need for SF2, and have SF2 define its own ops structure? We can then swap to const ops dsa wide. Thanks Andrew
Le 01/08/17 à 09:41, Andrew Lunn a écrit : > On Sat, Jan 07, 2017 at 09:01:56PM -0800, Florian Fainelli wrote: >> We make the bcm_sf2 driver override ds->ops which points to >> b53_switch_ops since b53_switch_alloc() did the assignent. This is all >> well and good until a second b53 switch comes in, and ends up using the >> bcm_sf2 operations. Make a proper local copy, substitute the ds->ops >> pointer and then override the operations. >> >> Fixes: f458995b9ad8 ("net: dsa: bcm_sf2: Utilize core B53 driver when possible") >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > Hi Florian Hi Andrew, > > There is a general trend of making ops structures const. It closes off > kernel exploits. This coping and then modifying prevents us making > ds->ops a pointer to a const. Agreed, and this was my initial approach, but I also wanted a minimal fix for David to pull into "net" while we can properly resolve this for "net-next" see below. > > You are already using b53_common.c as a library. Could you go further > with the concept, and export the ops you need for SF2, and have SF2 > define its own ops structure? We can then swap to const ops dsa wide. Making the ops const was my initial approach but there are several challenges to making it possible right now which I will address against net-next: - register/unregister_switch_driver actually do modify dsa_switch_ops while updating the list pointer, so we need to encapsulate dsa_switch_ops into a dsa_switch_driver plus a list member - as you pointed out, b53 needs to export the operations to other drivers that are going to make use of them Thanks for your comments!
> Agreed, and this was my initial approach, but I also wanted a minimal > fix for David to pull into "net" while we can properly resolve this for > "net-next" see below. O.K, so in that case, this is fine. > Making the ops const was my initial approach but there are several > challenges to making it possible right now which I will address against > net-next: > > - register/unregister_switch_driver actually do modify dsa_switch_ops > while updating the list pointer, so we need to encapsulate > dsa_switch_ops into a dsa_switch_driver plus a list member O.K, this is dsa v1. I had v2 in mind. Yes, the list needs abstracting. Thanks Andrew
On Sat, Jan 07, 2017 at 09:01:56PM -0800, Florian Fainelli wrote: > We make the bcm_sf2 driver override ds->ops which points to > b53_switch_ops since b53_switch_alloc() did the assignent. This is all > well and good until a second b53 switch comes in, and ends up using the > bcm_sf2 operations. Make a proper local copy, substitute the ds->ops > pointer and then override the operations. > > Fixes: f458995b9ad8 ("net: dsa: bcm_sf2: Utilize core B53 driver when possible") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c index 9ec33b51a0ed..2f9f910c0e40 100644 --- a/drivers/net/dsa/bcm_sf2.c +++ b/drivers/net/dsa/bcm_sf2.c @@ -982,6 +982,7 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev) const char *reg_names[BCM_SF2_REGS_NUM] = BCM_SF2_REGS_NAME; struct device_node *dn = pdev->dev.of_node; struct b53_platform_data *pdata; + struct dsa_switch_ops *ops; struct bcm_sf2_priv *priv; struct b53_device *dev; struct dsa_switch *ds; @@ -995,6 +996,10 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev) if (!priv) return -ENOMEM; + ops = devm_kzalloc(&pdev->dev, sizeof(*ops), GFP_KERNEL); + if (!ops) + return -ENOMEM; + dev = b53_switch_alloc(&pdev->dev, &bcm_sf2_io_ops, priv); if (!dev) return -ENOMEM; @@ -1014,6 +1019,8 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev) ds = dev->ds; /* Override the parts that are non-standard wrt. normal b53 devices */ + memcpy(ops, ds->ops, sizeof(*ops)); + ds->ops = ops; ds->ops->get_tag_protocol = bcm_sf2_sw_get_tag_protocol; ds->ops->setup = bcm_sf2_sw_setup; ds->ops->get_phy_flags = bcm_sf2_sw_get_phy_flags;
We make the bcm_sf2 driver override ds->ops which points to b53_switch_ops since b53_switch_alloc() did the assignent. This is all well and good until a second b53 switch comes in, and ends up using the bcm_sf2 operations. Make a proper local copy, substitute the ds->ops pointer and then override the operations. Fixes: f458995b9ad8 ("net: dsa: bcm_sf2: Utilize core B53 driver when possible") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/dsa/bcm_sf2.c | 7 +++++++ 1 file changed, 7 insertions(+)