Re: [PATCH 1/1] drivers:net:ethernet:adi:bfin_mac: Use FIELD_SIZEOF defined kernel macro

[Geert Uytterhoeven]

Hi Karim,

On Sun, Apr 23, 2017 at 8:02 PM, Karim Eshapa <karim.eshapa@xxxxxxxxx> wrote:
> Use FIELD_SIZEOF defined kernel macro kernel.h
>
> Signed-off-by: Karim Eshapa <karim.eshapa@xxxxxxxxx>
> ---
>  drivers/net/ethernet/adi/bfin_mac.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/adi/bfin_mac.c b/drivers/net/ethernet/adi/bfin_mac.c
> index a9ac58c..60346e0 100644
> --- a/drivers/net/ethernet/adi/bfin_mac.c
> +++ b/drivers/net/ethernet/adi/bfin_mac.c
> @@ -452,10 +452,14 @@ static irqreturn_t bfin_mac_wake_interrupt(int irq, void *dev_id)
>  static void bfin_mac_ethtool_getdrvinfo(struct net_device *dev,
>                                         struct ethtool_drvinfo *info)
>  {
> -       strlcpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
> -       strlcpy(info->version, DRV_VERSION, sizeof(info->version));
> -       strlcpy(info->fw_version, "N/A", sizeof(info->fw_version));
> -       strlcpy(info->bus_info, dev_name(&dev->dev), sizeof(info->bus_info));
> +       strlcpy(info->driver, KBUILD_MODNAME, FIELD_SIZEOF(
> +               struct ethtool_drvinfo, driver));

IMHO this makes the code less safe and less future-proof.
What if the type of info is ever changed?
There's no safety check to validate that the FIELD_SIZEOF() operates on the
same data as the strlcpy() destination.

> +       strlcpy(info->version, DRV_VERSION, FIELD_SIZEOF(
> +               struct ethtool_drvinfo, version));
> +       strlcpy(info->fw_version, "N/A", FIELD_SIZEOF(
> +               struct ethtool_drvinfo, fw_version));
> +       strlcpy(info->bus_info, dev_name(&dev->dev), FIELD_SIZEOF(
> +               struct ethtool_drvinfo, bus_info));
>  }
>
>  static void bfin_mac_ethtool_getwol(struct net_device *dev,
> @@ -785,7 +789,7 @@ static int bfin_mac_hwtstamp_get(struct net_device *netdev,
>         struct bfin_mac_local *lp = netdev_priv(netdev);
>
>         return copy_to_user(ifr->ifr_data, &lp->stamp_cfg,
> -                           sizeof(lp->stamp_cfg)) ?
> +                           FILD_SIZEOF(struct bfin_mac_local, stamp_cfg)) ?

As the kbuild test robot already told you, this doesn't compile.
Please try to (at least) compile the code before sending patches.
Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds