netdev - Re: [PATCH net-next-2.6] bridge: add _

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1289685861.2743.38.camel@edumazet-laptop>
Date:	Sat, 13 Nov 2010 23:04:21 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Stephen Hemminger <shemminger@...ux-foundation.org>
Cc:	David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next-2.6] bridge: add __rcu annotations

Le samedi 13 novembre 2010 à 10:13 -0800, Stephen Hemminger a écrit :
> On Sat, 13 Nov 2010 18:58:50 +0100
> Eric Dumazet <eric.dumazet@...il.com> wrote:
> 
> > Le samedi 13 novembre 2010 à 09:35 -0800, Stephen Hemminger a écrit :
> > > On Sat, 13 Nov 2010 09:15:28 +0100
> > > Eric Dumazet <eric.dumazet@...il.com> wrote:
> > > 
> > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > > index 578debb..ffbd177 100644
> > > > --- a/include/linux/netdevice.h
> > > > +++ b/include/linux/netdevice.h
> > > > @@ -996,7 +996,10 @@ struct net_device {
> > > >  #endif
> > > >  
> > > >  	rx_handler_func_t	*rx_handler;
> > > > -	void			*rx_handler_data;
> > > > +	union {
> > > > +		void				*rx_handler_data;
> > > > +		struct net_bridge_port __rcu	*br_port_rcu;
> > > > +	};
> > > >  
> > > >  	struct netdev_queue __rcu *ingress_queue;
> > > 
> > > I don't like making the generic hook typed again.
> > > We don't do this for other callbacks, timers, workqueues, ...
> > > Why is it necessary for RCU notation.
> > > 
> > 
> > because rcu_dereference() needs the type for __CHECKER__/sparse checks
> > 
> > #define __rcu_dereference_check(p, c, space) \
> >         ({ \
> >                 typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
> >                 rcu_lockdep_assert(c); \
> >                 rcu_dereference_sparse(p, space); \
> >                 smp_read_barrier_depends(); \
> >                 ((typeof(*p) __force __kernel *)(_________p1)); \
> >         })
> > 
> > So using a "void *ptr" is not an option
> > 
> > Its also cleaner to use
> > 
> > rcu_dereference(dev->br_port_rcu)
> > 
> > instead of 
> > 
> > (struct net_bridge_port *)rcu_dereference(dev->rx_handler_data)
> 
> There must be a better way. What about use of that hook by macvlan and openvswitch?

macvlan and openvswitch (is it part of linux yet ???)

I honestly dont understand your point Stephen, maybe you could explain a
bit more what is the problem ?

I use a union, like many other ones in the kernel. This is the first
time I ear this is not good to add type safety.

You can use either one or other field at your convenience.

If you are talking about stacking hooks, that has nothing to do with
this (cleanup) rcu patch, but previous introduction of
rx_handler_data/rx_handler ?

Please run sparse on x86_64 machine and watch all the warnings in bridge
code. (with CONFIG_SPARSE_RCU_POINTER=y)

Me confused.


--
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