netdev - Re: [PATCH net-next 1/2] net: inform NETDEV_CHANGE callbacks which flags were changed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1369326662.6963.4.camel@bwh-desktop.uk.level5networks.com>
Date:	Thu, 23 May 2013 17:31:02 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Timo Teräs <timo.teras@....fi>
CC:	<netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/2] net: inform NETDEV_CHANGE callbacks which
 flags were changed

On Thu, 2013-05-23 at 12:58 +0300, Timo Teräs wrote:
> In certain cases (like the follow up commit to arp.c) will need to
> check which flags actually changed to avoid excessive work.
> 
> Ben Hutchings nicely worded why to put these transient flags to
> struct net_device for the time being:
> > It's inelegant to put transient data associated with an event in a
> > persistent data structure.  On the other hand, having every user cache
> > the old state is pretty awful as well.
> >
> > Really, netdev notifiers should be changed to accept a structure that
> > encapsulates the changes rather than just a pointer to the net_device.
> > But making such a change would be an enormous pain and error-prone
> > because notifier functions aren't type-safe.
> >
> > As an interim solution, I think either the general priv_flags_changed or
> > old_priv_flags would be preferable to defining extra transient flags.
> 
> Signed-off-by: Timo Teräs <timo.teras@....fi>
> Cc: Ben Hutchings <bhutchings@...arflare.com>

Could you delete the two instances of 'priv_' from the quoted text?  I
was confused about to which flags field was being changed.  With that,
you may add 'Acked-by: Ben Hutchings <bhutchings@...arflare.com>'

Ben.

> ---
>  include/linux/netdevice.h | 4 +++-
>  net/core/dev.c            | 5 ++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7dd535d..86c155a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1112,7 +1112,9 @@ struct net_device {
>  	/* Hardware header description */
>  	const struct header_ops *header_ops;
>  
> -	unsigned int		flags;	/* interface flags (a la BSD)	*/
> +	unsigned int		flags;		/* interface flags (a la BSD)	*/
> +	unsigned int		flags_changed;	/* flags that are being changed
> +						 * valid during NETDEV_CHANGE notifier */
>  	unsigned int		priv_flags; /* Like 'flags' but invisible to userspace.
>  					     * See if.h for definitions. */
>  	unsigned short		gflags;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 7229bc3..221634f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4741,8 +4741,11 @@ void __dev_notify_flags(struct net_device *dev, unsigned int old_flags)
>  	}
>  
>  	if (dev->flags & IFF_UP &&
> -	    (changes & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI | IFF_VOLATILE)))
> +	    (changes & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI | IFF_VOLATILE))) {
> +		dev->flags_changed = changes;
>  		call_netdevice_notifiers(NETDEV_CHANGE, dev);
> +		dev->flags_changed = 0;
> +	}
>  }
>  
>  /**

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html