Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
On 11/09/2019 11.16, Dan Carpenter wrote: > On Wed, Sep 11, 2019 at 11:04:38AM +0200, Sandro Volery wrote: >> >> >>> On 11 Sep 2019, at 10:52, Dan Carpenter wrote: >>> >>> On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: strcpy was used multiple times in strcpy to write into dev->name. I replaced them with strscpy. Yes, that's obviously what the patch does. The commit log is supposed to explain _why_. Signed-off-by: Sandro Volery --- drivers/staging/octeon/ethernet.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c index 8889494adf1f..cf8e9a23ebf9 100644 --- a/drivers/staging/octeon/ethernet.c +++ b/drivers/staging/octeon/ethernet.c @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; priv->port = CVMX_PIP_NUM_INPUT_PORTS; priv->queue = -1; -strcpy(dev->name, "pow%d"); +strscpy(dev->name, "pow%d", sizeof(dev->name)); >>> >>> Is there a program which is generating a warning for this code? We know >>> that "pow%d" is 6 characters and static analysis tools can understand >>> this code fine so we know it's safe. >> >> Well I was confused too but checkpatch complained about >> it so I figured I'd clean it up quick > > Ah. It's a new checkpatch warning. I don't care in that case. I'm > fine with replacing all of these in that case. But why? It actually gives _less_ compile-time checking (gcc and all static tools know perfectly well what strcpy is and does, but knows nothing of strscpy). And using sizeof() instead of ARRAY_SIZE() means a future reader is not even sure dev->name is not just a pointer. Moreover, it's very likely also a runtime and .text pessimization, again because gcc knows what strcpy does, so it can just do a few immediate stores (e.g. 0x25776f70 for the "pow%" part) instead of emitting an actual function call. Rasmus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
On Wed, Sep 11, 2019 at 11:21:44AM +0200, Sandro Volery wrote: > > On 11 Sep 2019, at 11:17, Dan Carpenter wrote: > > > > On Wed, Sep 11, 2019 at 11:04:38AM +0200, Sandro Volery wrote: > >> > >> > On 11 Sep 2019, at 10:52, Dan Carpenter wrote: > >>> > >>> On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: > strcpy was used multiple times in strcpy to write into dev->name. > I replaced them with strscpy. > > Signed-off-by: Sandro Volery > --- > drivers/staging/octeon/ethernet.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/staging/octeon/ethernet.c > b/drivers/staging/octeon/ethernet.c > index 8889494adf1f..cf8e9a23ebf9 100644 > --- a/drivers/staging/octeon/ethernet.c > +++ b/drivers/staging/octeon/ethernet.c > @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device > *pdev) > priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; > priv->port = CVMX_PIP_NUM_INPUT_PORTS; > priv->queue = -1; > -strcpy(dev->name, "pow%d"); > +strscpy(dev->name, "pow%d", sizeof(dev->name)); > >>> > >>> Is there a program which is generating a warning for this code? We know > >>> that "pow%d" is 6 characters and static analysis tools can understand > >>> this code fine so we know it's safe. > >> > >> Well I was confused too but checkpatch complained about > >> it so I figured I'd clean it up quick > > > > Ah. It's a new checkpatch warning. I don't care in that case. I'm > > fine with replacing all of these in that case. > > Alright thanks. Can you review this? > Sure. Reviewed-by: Dan Carpenter regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
On 11 Sep 2019, at 11:17, Dan Carpenter wrote: > > On Wed, Sep 11, 2019 at 11:04:38AM +0200, Sandro Volery wrote: >> >> On 11 Sep 2019, at 10:52, Dan Carpenter wrote: >>> >>> On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: strcpy was used multiple times in strcpy to write into dev->name. I replaced them with strscpy. Signed-off-by: Sandro Volery --- drivers/staging/octeon/ethernet.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c index 8889494adf1f..cf8e9a23ebf9 100644 --- a/drivers/staging/octeon/ethernet.c +++ b/drivers/staging/octeon/ethernet.c @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; priv->port = CVMX_PIP_NUM_INPUT_PORTS; priv->queue = -1; -strcpy(dev->name, "pow%d"); +strscpy(dev->name, "pow%d", sizeof(dev->name)); >>> >>> Is there a program which is generating a warning for this code? We know >>> that "pow%d" is 6 characters and static analysis tools can understand >>> this code fine so we know it's safe. >> >> Well I was confused too but checkpatch complained about >> it so I figured I'd clean it up quick > > Ah. It's a new checkpatch warning. I don't care in that case. I'm > fine with replacing all of these in that case. Alright thanks. Can you review this? Thanks, Sandro V ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
On Wed, Sep 11, 2019 at 11:04:38AM +0200, Sandro Volery wrote: > > > > On 11 Sep 2019, at 10:52, Dan Carpenter wrote: > > > > On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: > >> strcpy was used multiple times in strcpy to write into dev->name. > >> I replaced them with strscpy. > >> > >> Signed-off-by: Sandro Volery > >> --- > >> drivers/staging/octeon/ethernet.c | 16 > >> 1 file changed, 8 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/staging/octeon/ethernet.c > >> b/drivers/staging/octeon/ethernet.c > >> index 8889494adf1f..cf8e9a23ebf9 100644 > >> --- a/drivers/staging/octeon/ethernet.c > >> +++ b/drivers/staging/octeon/ethernet.c > >> @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) > >>priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; > >>priv->port = CVMX_PIP_NUM_INPUT_PORTS; > >>priv->queue = -1; > >> -strcpy(dev->name, "pow%d"); > >> +strscpy(dev->name, "pow%d", sizeof(dev->name)); > > > > Is there a program which is generating a warning for this code? We know > > that "pow%d" is 6 characters and static analysis tools can understand > > this code fine so we know it's safe. > > Well I was confused too but checkpatch complained about > it so I figured I'd clean it up quick Ah. It's a new checkpatch warning. I don't care in that case. I'm fine with replacing all of these in that case. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
> On 11 Sep 2019, at 10:52, Dan Carpenter wrote: > > On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: >> strcpy was used multiple times in strcpy to write into dev->name. >> I replaced them with strscpy. >> >> Signed-off-by: Sandro Volery >> --- >> drivers/staging/octeon/ethernet.c | 16 >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/staging/octeon/ethernet.c >> b/drivers/staging/octeon/ethernet.c >> index 8889494adf1f..cf8e9a23ebf9 100644 >> --- a/drivers/staging/octeon/ethernet.c >> +++ b/drivers/staging/octeon/ethernet.c >> @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) >>priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; >>priv->port = CVMX_PIP_NUM_INPUT_PORTS; >>priv->queue = -1; >> -strcpy(dev->name, "pow%d"); >> +strscpy(dev->name, "pow%d", sizeof(dev->name)); > > Is there a program which is generating a warning for this code? We know > that "pow%d" is 6 characters and static analysis tools can understand > this code fine so we know it's safe. Well I was confused too but checkpatch complained about it so I figured I'd clean it up quick ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: > strcpy was used multiple times in strcpy to write into dev->name. > I replaced them with strscpy. > > Signed-off-by: Sandro Volery > --- > drivers/staging/octeon/ethernet.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/staging/octeon/ethernet.c > b/drivers/staging/octeon/ethernet.c > index 8889494adf1f..cf8e9a23ebf9 100644 > --- a/drivers/staging/octeon/ethernet.c > +++ b/drivers/staging/octeon/ethernet.c > @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) > priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; > priv->port = CVMX_PIP_NUM_INPUT_PORTS; > priv->queue = -1; > - strcpy(dev->name, "pow%d"); > + strscpy(dev->name, "pow%d", sizeof(dev->name)); Is there a program which is generating a warning for this code? We know that "pow%d" is 6 characters and static analysis tools can understand this code fine so we know it's safe. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Staging: octeon: Avoid several usecases of strcpy
strcpy was used multiple times in strcpy to write into dev->name. I replaced them with strscpy. Signed-off-by: Sandro Volery --- drivers/staging/octeon/ethernet.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c index 8889494adf1f..cf8e9a23ebf9 100644 --- a/drivers/staging/octeon/ethernet.c +++ b/drivers/staging/octeon/ethernet.c @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; priv->port = CVMX_PIP_NUM_INPUT_PORTS; priv->queue = -1; - strcpy(dev->name, "pow%d"); + strscpy(dev->name, "pow%d", sizeof(dev->name)); for (qos = 0; qos < 16; qos++) skb_queue_head_init(>tx_free_list[qos]); dev->min_mtu = VLAN_ETH_ZLEN - mtu_overhead; @@ -856,39 +856,39 @@ static int cvm_oct_probe(struct platform_device *pdev) case CVMX_HELPER_INTERFACE_MODE_NPI: dev->netdev_ops = _oct_npi_netdev_ops; - strcpy(dev->name, "npi%d"); + strscpy(dev->name, "npi%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_XAUI: dev->netdev_ops = _oct_xaui_netdev_ops; - strcpy(dev->name, "xaui%d"); + strscpy(dev->name, "xaui%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_LOOP: dev->netdev_ops = _oct_npi_netdev_ops; - strcpy(dev->name, "loop%d"); + strscpy(dev->name, "loop%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_SGMII: priv->phy_mode = PHY_INTERFACE_MODE_SGMII; dev->netdev_ops = _oct_sgmii_netdev_ops; - strcpy(dev->name, "eth%d"); + strscpy(dev->name, "eth%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_SPI: dev->netdev_ops = _oct_spi_netdev_ops; - strcpy(dev->name, "spi%d"); + strscpy(dev->name, "spi%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_GMII: priv->phy_mode = PHY_INTERFACE_MODE_GMII; dev->netdev_ops = _oct_rgmii_netdev_ops; - strcpy(dev->name, "eth%d"); + strscpy(dev->name, "eth%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_RGMII: dev->netdev_ops = _oct_rgmii_netdev_ops; - strcpy(dev->name, "eth%d"); + strscpy(dev->name, "eth%d", sizeof(dev->name)); cvm_set_rgmii_delay(priv, interface, port_index); break; -- 2.23.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
