Linux Ethernet Bridging - Dec 2013 - [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

br_stp_rcv() is reached by non-rx_handler path. That means there is no
guarantee that dev is bridge port and therefore simple NULL check of
->rx_handler_data is not enough. There is need to check if dev is really
bridge port and since only rcu read lock is held here, do it by checking
->rx_handler pointer.

Note that synchronize_net() in netdev_rx_handler_unregister() ensures
this approach as valid.

Introduced originally by:
commit f350a0a87374418635689471606454abc7beaa3a
  "bridge: use rx_handler_data pointer to store net_bridge_port
pointer"

Fixed but not in the best way by:
commit b5ed54e94d324f17c97852296d61a143f01b227a
  "bridge: fix RCU races with bridge port"

Reintroduced by:
commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2
  "bridge: fix NULL pointer deref of br_port_get_rcu"

Please apply to stable trees as well. Thanks.

RH bugzilla reference: https://bugzilla.redhat.com/show_bug.cgi?id=1025770

Reported-by: Laine Stump <laine@redhat.com>
Debugged-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
v1->v2: moved br_port_get_check_rcu definition below br_handle_frame
definition

 net/bridge/br_private.h  | 10 ++++++++++
 net/bridge/br_stp_bpdu.c |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 229d820..045d56e 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -426,6 +426,16 @@ netdev_features_t br_features_recompute(struct net_bridge
*br,
 int br_handle_frame_finish(struct sk_buff *skb);
 rx_handler_result_t br_handle_frame(struct sk_buff **pskb);
 
+static inline bool br_rx_handler_check_rcu(const struct net_device *dev)
+{
+	return rcu_dereference(dev->rx_handler) == br_handle_frame;
+}
+
+static inline struct net_bridge_port *br_port_get_check_rcu(const struct
net_device *dev)
+{
+	return br_rx_handler_check_rcu(dev) ? br_port_get_rcu(dev) : NULL;
+}
+
 /* br_ioctl.c */
 int br_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd);
 int br_ioctl_deviceless_stub(struct net *net, unsigned int cmd,
diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
index 8660ea3..bdb459d 100644
--- a/net/bridge/br_stp_bpdu.c
+++ b/net/bridge/br_stp_bpdu.c
@@ -153,7 +153,7 @@ void br_stp_rcv(const struct stp_proto *proto, struct
sk_buff *skb,
 	if (buf[0] != 0 || buf[1] != 0 || buf[2] != 0)
 		goto err;
 
-	p = br_port_get_rcu(dev);
+	p = br_port_get_check_rcu(dev);
 	if (!p)
 		goto err;
 
-- 
1.8.3.1

On Sat, 7 Dec 2013 09:51:05 +0100
Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Dec 06, 2013 at 10:10:28PM CET, stephen@networkplumber.org wrote:
> >On Fri, 06 Dec 2013 15:43:21 -0500 (EST)
> >David Miller <davem@davemloft.net> wrote:
> >
> >> From: Jiri Pirko <jiri@resnulli.us>
> >> Date: Thu,  5 Dec 2013 16:27:37 +0100
> >> 
> >> > br_stp_rcv() is reached by non-rx_handler path. That means
there is no
> >> > guarantee that dev is bridge port and therefore simple NULL
check of
> >> > ->rx_handler_data is not enough. There is need to check if
dev is really
> >> > bridge port and since only rcu read lock is held here, do it
by checking
> >> > ->rx_handler pointer.
> >> > 
> >> > Note that synchronize_net() in netdev_rx_handler_unregister()
ensures
> >> > this approach as valid.
> >> > 
> >> > Introduced originally by:
> >> > commit f350a0a87374418635689471606454abc7beaa3a
> >> >   "bridge: use rx_handler_data pointer to store
net_bridge_port pointer"
> >> > 
> >> > Fixed but not in the best way by:
> >> > commit b5ed54e94d324f17c97852296d61a143f01b227a
> >> >   "bridge: fix RCU races with bridge port"
> >> > 
> >> > Reintroduced by:
> >> > commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2
> >> >   "bridge: fix NULL pointer deref of
br_port_get_rcu"
> >> > 
> >> > Please apply to stable trees as well. Thanks.
> >> > 
> >> > RH bugzilla reference:
https://bugzilla.redhat.com/show_bug.cgi?id=1025770
> >> > 
> >> > Reported-by: Laine Stump <laine@redhat.com>
> >> > Debugged-by: Michael S. Tsirkin <mst@redhat.com>
> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> > Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >> > ---
> >> > v1->v2: moved br_port_get_check_rcu definition below
br_handle_frame definition
> >> 
> >> Applied and queued up for -stable, thanks Jiri.
> >
> >How come you ignored my simpler fix, that used the existing logic.
> >I don''t like introducing this especially into the stable; much
prefer
> >to go back to testing the flag as was being done before.
> 
> Although your patch is technically sane, it depends on rtnl indirectly.
> My patch depends on rcu locking and synchronize_rcu which is direct.
> Therefore I think it is more appropriate.
After more review and thought I agree. But could we put some comments
in br_private.h to describe the dependency on ordering (synchronize_net).

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

On 12/07/2013 03:51 AM, Jiri Pirko wrote:
> Fri, Dec 06, 2013 at 10:10:28PM CET, stephen@networkplumber.org wrote:
>> On Fri, 06 Dec 2013 15:43:21 -0500 (EST)
>> David Miller <davem@davemloft.net> wrote:
>>
>>> From: Jiri Pirko <jiri@resnulli.us>
>>> Date: Thu,  5 Dec 2013 16:27:37 +0100
>>>
>>>> br_stp_rcv() is reached by non-rx_handler path. That means
there is no
>>>> guarantee that dev is bridge port and therefore simple NULL
check of
>>>> ->rx_handler_data is not enough. There is need to check if
dev is really
>>>> bridge port and since only rcu read lock is held here, do it by
checking
>>>> ->rx_handler pointer.
>>>>
>>>> Note that synchronize_net() in netdev_rx_handler_unregister()
ensures
>>>> this approach as valid.
>>>>
>>>> Introduced originally by:
>>>> commit f350a0a87374418635689471606454abc7beaa3a
>>>>   "bridge: use rx_handler_data pointer to store
net_bridge_port pointer"
>>>>
>>>> Fixed but not in the best way by:
>>>> commit b5ed54e94d324f17c97852296d61a143f01b227a
>>>>   "bridge: fix RCU races with bridge port"
>>>>
>>>> Reintroduced by:
>>>> commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2
>>>>   "bridge: fix NULL pointer deref of br_port_get_rcu"
>>>>
>>>> Please apply to stable trees as well. Thanks.
>>>>
>>>> RH bugzilla reference:
https://bugzilla.redhat.com/show_bug.cgi?id=1025770
>>>>
>>>> Reported-by: Laine Stump <laine@redhat.com>
>>>> Debugged-by: Michael S. Tsirkin <mst@redhat.com>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>>> ---
>>>> v1->v2: moved br_port_get_check_rcu definition below
br_handle_frame definition
>>>
>>> Applied and queued up for -stable, thanks Jiri.
>>
>> How come you ignored my simpler fix, that used the existing logic.
>> I don''t like introducing this especially into the stable; much
prefer
>> to go back to testing the flag as was being done before.
> 
> Although your patch is technically sane, it depends on rtnl indirectly.
Pardon my ignorance, but I''ve been staring at this and I can''t
for
the life of me see the dependency.

The IFF_BRIDGE_PORT flag is set after the rx_handler is registered,
so we are safe there.  The rcu primitives will guarantee that the flag
will be set by the time rx_handler and rx_handler_data are set.

The flag is cleared before rx_handler is unregistered, so it is
still valid to check for it in stp code.  Once the flag is cleared
we may still have a valid rx_handler during the rcu grace period, but
will still avoid doing processing.

So, where is the dependency on the rtnl?

Thanks
-vlad
> My patch depends on rcu locking and synchronize_rcu which is direct.
> Therefore I think it is more appropriate.
> 
> Jiri
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev"
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Sat, Dec 07, 2013 at 08:10:45PM CET, vyasevich@gmail.com
wrote:
>On 12/07/2013 03:51 AM, Jiri Pirko wrote:
>> Fri, Dec 06, 2013 at 10:10:28PM CET, stephen@networkplumber.org wrote:
>>> On Fri, 06 Dec 2013 15:43:21 -0500 (EST)
>>> David Miller <davem@davemloft.net> wrote:
>>>
>>>> From: Jiri Pirko <jiri@resnulli.us>
>>>> Date: Thu,  5 Dec 2013 16:27:37 +0100
>>>>
>>>>> br_stp_rcv() is reached by non-rx_handler path. That means
there is no
>>>>> guarantee that dev is bridge port and therefore simple NULL
check of
>>>>> ->rx_handler_data is not enough. There is need to check
if dev is really
>>>>> bridge port and since only rcu read lock is held here, do
it by checking
>>>>> ->rx_handler pointer.
>>>>>
>>>>> Note that synchronize_net() in
netdev_rx_handler_unregister() ensures
>>>>> this approach as valid.
>>>>>
>>>>> Introduced originally by:
>>>>> commit f350a0a87374418635689471606454abc7beaa3a
>>>>>   "bridge: use rx_handler_data pointer to store
net_bridge_port pointer"
>>>>>
>>>>> Fixed but not in the best way by:
>>>>> commit b5ed54e94d324f17c97852296d61a143f01b227a
>>>>>   "bridge: fix RCU races with bridge port"
>>>>>
>>>>> Reintroduced by:
>>>>> commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2
>>>>>   "bridge: fix NULL pointer deref of
br_port_get_rcu"
>>>>>
>>>>> Please apply to stable trees as well. Thanks.
>>>>>
>>>>> RH bugzilla reference:
https://bugzilla.redhat.com/show_bug.cgi?id=1025770
>>>>>
>>>>> Reported-by: Laine Stump <laine@redhat.com>
>>>>> Debugged-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>>>> ---
>>>>> v1->v2: moved br_port_get_check_rcu definition below
br_handle_frame definition
>>>>
>>>> Applied and queued up for -stable, thanks Jiri.
>>>
>>> How come you ignored my simpler fix, that used the existing logic.
>>> I don''t like introducing this especially into the stable;
much prefer
>>> to go back to testing the flag as was being done before.
>> 
>> Although your patch is technically sane, it depends on rtnl indirectly.
>
>Pardon my ignorance, but I''ve been staring at this and I
can''t for
>the life of me see the dependency.
>
>The IFF_BRIDGE_PORT flag is set after the rx_handler is registered,
>so we are safe there.  The rcu primitives will guarantee that the flag
>will be set by the time rx_handler and rx_handler_data are set.
>
>The flag is cleared before rx_handler is unregistered, so it is
>still valid to check for it in stp code.  Once the flag is cleared
>we may still have a valid rx_handler during the rcu grace period, but
>will still avoid doing processing.
>
>So, where is the dependency on the rtnl?

Imagine br would release the netdev and some other rx_handler user would
enslave the same netdev. This two events would happen between
IFF_BRIDGE_PORT flag check and rx_handler_data get. That is what
rtnl_lock prevents from happening.
>
>Thanks
>-vlad
>
>> My patch depends on rcu locking and synchronize_rcu which is direct.
>> Therefore I think it is more appropriate.
>> 
>> Jiri
>> 
>> 
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe
netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>

On 12/07/2013 03:07 PM, Jiri Pirko wrote:
> Sat, Dec 07, 2013 at 08:10:45PM CET, vyasevich@gmail.com wrote:
>> On 12/07/2013 03:51 AM, Jiri Pirko wrote:
>>> Fri, Dec 06, 2013 at 10:10:28PM CET, stephen@networkplumber.org
wrote:
>>>> On Fri, 06 Dec 2013 15:43:21 -0500 (EST)
>>>> David Miller <davem@davemloft.net> wrote:
>>>>
>>>>> From: Jiri Pirko <jiri@resnulli.us>
>>>>> Date: Thu,  5 Dec 2013 16:27:37 +0100
>>>>>
>>>>>> br_stp_rcv() is reached by non-rx_handler path. That
means there is no
>>>>>> guarantee that dev is bridge port and therefore simple
NULL check of
>>>>>> ->rx_handler_data is not enough. There is need to
check if dev is really
>>>>>> bridge port and since only rcu read lock is held here,
do it by checking
>>>>>> ->rx_handler pointer.
>>>>>>
>>>>>> Note that synchronize_net() in
netdev_rx_handler_unregister() ensures
>>>>>> this approach as valid.
>>>>>>
>>>>>> Introduced originally by:
>>>>>> commit f350a0a87374418635689471606454abc7beaa3a
>>>>>>   "bridge: use rx_handler_data pointer to store
net_bridge_port pointer"
>>>>>>
>>>>>> Fixed but not in the best way by:
>>>>>> commit b5ed54e94d324f17c97852296d61a143f01b227a
>>>>>>   "bridge: fix RCU races with bridge port"
>>>>>>
>>>>>> Reintroduced by:
>>>>>> commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2
>>>>>>   "bridge: fix NULL pointer deref of
br_port_get_rcu"
>>>>>>
>>>>>> Please apply to stable trees as well. Thanks.
>>>>>>
>>>>>> RH bugzilla reference:
https://bugzilla.redhat.com/show_bug.cgi?id=1025770
>>>>>>
>>>>>> Reported-by: Laine Stump <laine@redhat.com>
>>>>>> Debugged-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>> Signed-off-by: Michael S. Tsirkin
<mst@redhat.com>
>>>>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>>>>> ---
>>>>>> v1->v2: moved br_port_get_check_rcu definition below
br_handle_frame definition
>>>>>
>>>>> Applied and queued up for -stable, thanks Jiri.
>>>>
>>>> How come you ignored my simpler fix, that used the existing
logic.
>>>> I don''t like introducing this especially into the
stable; much prefer
>>>> to go back to testing the flag as was being done before.
>>>
>>> Although your patch is technically sane, it depends on rtnl
indirectly.
>>
>> Pardon my ignorance, but I''ve been staring at this and I
can''t for
>> the life of me see the dependency.
>>
>> The IFF_BRIDGE_PORT flag is set after the rx_handler is registered,
>> so we are safe there.  The rcu primitives will guarantee that the flag
>> will be set by the time rx_handler and rx_handler_data are set.
>>
>> The flag is cleared before rx_handler is unregistered, so it is
>> still valid to check for it in stp code.  Once the flag is cleared
>> we may still have a valid rx_handler during the rcu grace period, but
>> will still avoid doing processing.
>>
>> So, where is the dependency on the rtnl?
> 
> 
> Imagine br would release the netdev and some other rx_handler user would
> enslave the same netdev. This two events would happen between
> IFF_BRIDGE_PORT flag check and rx_handler_data get. That is what
> rtnl_lock prevents from happening.
I don''t think this can happen.  Inside br_stp_rcv(), we are in an rcu
critical section.  The release of the netdev (rx_handler unregister)
forces us to to wait until synchronize_net() completes.  So, if under
rcu we checked the flag and it''s on, the rx_handler will not change for
the duration of that rcu section and we can safely process the packet
even if the port is in the middle of going away.  Howe does the race
you describe happen?

The reason I ask, is that we check priv_flags under rcu in other
places to make sure that the device we are passing data to can handle
it.  If it''s not safe, then those other places are vulnerable too.
It doesn''t seem to me that it is unsafe.

Thanks
-vlad
> 
>>
>> Thanks
>> -vlad
>>
>>> My patch depends on rcu locking and synchronize_rcu which is
direct.
>>> Therefore I think it is more appropriate.
>>>
>>> Jiri
>>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe
netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>

Mon, Dec 09, 2013 at 03:07:18AM CET, vyasevich@gmail.com
wrote:
>On 12/07/2013 03:07 PM, Jiri Pirko wrote:
>> Sat, Dec 07, 2013 at 08:10:45PM CET, vyasevich@gmail.com wrote:
>>> On 12/07/2013 03:51 AM, Jiri Pirko wrote:
>>>> Fri, Dec 06, 2013 at 10:10:28PM CET, stephen@networkplumber.org
wrote:
>>>>> On Fri, 06 Dec 2013 15:43:21 -0500 (EST)
>>>>> David Miller <davem@davemloft.net> wrote:
>>>>>
>>>>>> From: Jiri Pirko <jiri@resnulli.us>
>>>>>> Date: Thu,  5 Dec 2013 16:27:37 +0100
>>>>>>
>>>>>>> br_stp_rcv() is reached by non-rx_handler path.
That means there is no
>>>>>>> guarantee that dev is bridge port and therefore
simple NULL check of
>>>>>>> ->rx_handler_data is not enough. There is need
to check if dev is really
>>>>>>> bridge port and since only rcu read lock is held
here, do it by checking
>>>>>>> ->rx_handler pointer.
>>>>>>>
>>>>>>> Note that synchronize_net() in
netdev_rx_handler_unregister() ensures
>>>>>>> this approach as valid.
>>>>>>>
>>>>>>> Introduced originally by:
>>>>>>> commit f350a0a87374418635689471606454abc7beaa3a
>>>>>>>   "bridge: use rx_handler_data pointer to
store net_bridge_port pointer"
>>>>>>>
>>>>>>> Fixed but not in the best way by:
>>>>>>> commit b5ed54e94d324f17c97852296d61a143f01b227a
>>>>>>>   "bridge: fix RCU races with bridge
port"
>>>>>>>
>>>>>>> Reintroduced by:
>>>>>>> commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2
>>>>>>>   "bridge: fix NULL pointer deref of
br_port_get_rcu"
>>>>>>>
>>>>>>> Please apply to stable trees as well. Thanks.
>>>>>>>
>>>>>>> RH bugzilla reference:
https://bugzilla.redhat.com/show_bug.cgi?id=1025770
>>>>>>>
>>>>>>> Reported-by: Laine Stump <laine@redhat.com>
>>>>>>> Debugged-by: Michael S. Tsirkin
<mst@redhat.com>
>>>>>>> Signed-off-by: Michael S. Tsirkin
<mst@redhat.com>
>>>>>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>>>>>> ---
>>>>>>> v1->v2: moved br_port_get_check_rcu definition
below br_handle_frame definition
>>>>>>
>>>>>> Applied and queued up for -stable, thanks Jiri.
>>>>>
>>>>> How come you ignored my simpler fix, that used the existing
logic.
>>>>> I don''t like introducing this especially into the
stable; much prefer
>>>>> to go back to testing the flag as was being done before.
>>>>
>>>> Although your patch is technically sane, it depends on rtnl
indirectly.
>>>
>>> Pardon my ignorance, but I''ve been staring at this and I
can''t for
>>> the life of me see the dependency.
>>>
>>> The IFF_BRIDGE_PORT flag is set after the rx_handler is registered,
>>> so we are safe there.  The rcu primitives will guarantee that the
flag
>>> will be set by the time rx_handler and rx_handler_data are set.
>>>
>>> The flag is cleared before rx_handler is unregistered, so it is
>>> still valid to check for it in stp code.  Once the flag is cleared
>>> we may still have a valid rx_handler during the rcu grace period,
but
>>> will still avoid doing processing.
>>>
>>> So, where is the dependency on the rtnl?
>> 
>> 
>> Imagine br would release the netdev and some other rx_handler user
would
>> enslave the same netdev. This two events would happen between
>> IFF_BRIDGE_PORT flag check and rx_handler_data get. That is what
>> rtnl_lock prevents from happening.
>
>I don''t think this can happen.  Inside br_stp_rcv(), we are in an
rcu
>critical section.  The release of the netdev (rx_handler unregister)
>forces us to to wait until synchronize_net() completes.  So, if under
>rcu we checked the flag and it''s on, the rx_handler will not change
for
>the duration of that rcu section and we can safely process the packet
>even if the port is in the middle of going away.  Howe does the race
>you describe happen?
You are right. It cannot happen.
>
>The reason I ask, is that we check priv_flags under rcu in other
>places to make sure that the device we are passing data to can handle
>it.  If it''s not safe, then those other places are vulnerable too.
>It doesn''t seem to me that it is unsafe.
>
>Thanks
>-vlad
>
>> 
>>>
>>> Thanks
>>> -vlad
>>>
>>>> My patch depends on rcu locking and synchronize_rcu which is
direct.
>>>> Therefore I think it is more appropriate.
>>>>
>>>> Jiri
>>>>
>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe
netdev" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at 
http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>

Mon, Dec 09, 2013 at 12:58:35PM CET, mst@redhat.com
wrote:
>On Thu, Dec 05, 2013 at 04:27:37PM +0100, Jiri Pirko wrote:
>> br_stp_rcv() is reached by non-rx_handler path. That means there is no
>> guarantee that dev is bridge port and therefore simple NULL check of
>> ->rx_handler_data is not enough. There is need to check if dev is
really
>> bridge port and since only rcu read lock is held here, do it by
checking
>> ->rx_handler pointer.
>> 
>> Note that synchronize_net() in netdev_rx_handler_unregister() ensures
>> this approach as valid.
>> 
>> Introduced originally by:
>> commit f350a0a87374418635689471606454abc7beaa3a
>>   "bridge: use rx_handler_data pointer to store net_bridge_port
pointer"
>> 
>> Fixed but not in the best way by:
>> commit b5ed54e94d324f17c97852296d61a143f01b227a
>>   "bridge: fix RCU races with bridge port"
>> 
>> Reintroduced by:
>> commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2
>>   "bridge: fix NULL pointer deref of br_port_get_rcu"
>> 
>> Please apply to stable trees as well. Thanks.
>> 
>> RH bugzilla reference:
https://bugzilla.redhat.com/show_bug.cgi?id=1025770
>> 
>> Reported-by: Laine Stump <laine@redhat.com>
>> Debugged-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>> v1->v2: moved br_port_get_check_rcu definition below br_handle_frame
definition
>> 
>>  net/bridge/br_private.h  | 10 ++++++++++
>>  net/bridge/br_stp_bpdu.c |  2 +-
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 229d820..045d56e 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -426,6 +426,16 @@ netdev_features_t br_features_recompute(struct
net_bridge *br,
>>  int br_handle_frame_finish(struct sk_buff *skb);
>>  rx_handler_result_t br_handle_frame(struct sk_buff **pskb);
>>  
>> +static inline bool br_rx_handler_check_rcu(const struct net_device
*dev)
>> +{
>> +	return rcu_dereference(dev->rx_handler) == br_handle_frame;
>
>Actually this started to bother me.
>rcu_dereference is for when we dereference, isn''t it?
>I think we should use rcu_access_pointer here.
Yes. That can be done. That would safe a barrier on some archs.
>
>
>> +}
>
>
>Given all the confusion, how about we create an API to
>access rx handler data outside rx handler itself in a
>safe, documented way?
>
>If everyone agrees, we can then re-implement
>br_port_get_check_rcu on top of this API.
>
>What do others think?
I like this a lot.

Acked-by: Jiri Pirko <jiri@resnulli.us>
>
>---
>
>netdevice: allow access to rx_handler_data outside rx handler
>
>rx_handler_data is easy to use correctly within
>rx handler itself. Outside of that context, one must
>validate the handler first.
>
>Add an API to do this in a uniform way.
>
>Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>-->
>
>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>index 7f0ed42..7a353b1 100644
>--- a/include/linux/netdevice.h
>+++ b/include/linux/netdevice.h
>@@ -1320,6 +1320,9 @@ struct net_device {
> #endif
> 
> 	rx_handler_func_t __rcu	*rx_handler;
>+	/* rx_handler itself can use rx_handler_data directly.
>+	 * Others must use netdev_rx_handler_data_rcu_dereference.
>+	 */
> 	void __rcu		*rx_handler_data;
> 
> 	struct netdev_queue __rcu *ingress_queue;
>@@ -2399,6 +2402,31 @@ int netdev_rx_handler_register(struct net_device
*dev,
> 			       void *rx_handler_data);
> void netdev_rx_handler_unregister(struct net_device *dev);
> 
>+/**
>+ *	netdev_rx_handler_data_rcu_dereference - access receive handler data
>+ *	@dev: device to get handler data for
>+ *	@rx_handler: receive handler used to register this data
>+ *
>+ *	Check that the receive handler is valid for the device.
>+ *	Return handler data if it is, NULL otherwise.
>+ *
>+ *	Use this function if you want to access rx handler data
>+ *	outside rx handler itself.
>+ *
>+ *	The caller must invoke this function under RCU read lock.
>+ *
>+ *	For a general description of rx_handler, see enum rx_handler_result.
>+ */
>+static inline
>+void *netdev_rx_handler_data_rcu_dereference(struct net_device *dev,
>+					     rx_handler_func_t *rx_handler)
>+{
>+	if (rcu_access_pointer(dev->rx_handler) != rx_handler)
>+		return NULL;
>+
>+	return rcu_dereference(dev->rx_handler_data);
>+}
>+
> bool dev_valid_name(const char *name);
> int dev_ioctl(struct net *net, unsigned int cmd, void __user *);
> int dev_ethtool(struct net *net, struct ifreq *);