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

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

[Jiri Pirko]

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

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Michael S. Tsirkin]

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>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

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

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Eric Dumazet]

On Thu, 2013-12-05 at 16:27 +0100, Jiri Pirko wrote:

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

Acked-by: Eric Dumazet <edumazet@google.com>

Thanks

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Stephen Hemminger]

On Thu,  5 Dec 2013 16:27:37 +0100
Jiri Pirko <jiri@resnulli.us> 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.
> 


I think this patch is simpler/better, it restores the old logic.

Ps. submitting patches to bugzilla is a good way to have them ignored.

>From Stephen Hemminger <stephen@networkplumber.org>

Check that incoming STP packet is received on a port assigned to bridge
before processing. It is possible to receive packet on non-bridge port
because they are multicast.

See:
 https://bugzilla.kernel.org/show_bug.cgi?id=64911


Regression introduced by:
commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2
Author: Hong Zhiguo <zhiguohong@tencent.com>
Date:   Sat Sep 14 22:42:28 2013 +0800

    bridge: fix NULL pointer deref of br_port_get_rcu


Reported-by: Alexander Y. Fomichev
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>


--- a/net/bridge/br_stp_bpdu.c	2013-06-11 09:50:21.522919061 -0700
+++ b/net/bridge/br_stp_bpdu.c	2013-12-05 08:46:56.090463702 -0800
@@ -153,6 +153,9 @@ void br_stp_rcv(const struct stp_proto *
 	if (buf[0] != 0 || buf[1] != 0 || buf[2] != 0)
 		goto err;
 
+	if (!br_port_exists(dev))
+		goto err;
+
 	p = br_port_get_rcu(dev);
 	if (!p)
 		goto err;

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Gao feng]

On 12/06/2013 12:55 AM, Stephen Hemminger wrote:
> On Thu,  5 Dec 2013 16:27:37 +0100
> Jiri Pirko <jiri@resnulli.us> 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.
>>
> 
> 
> I think this patch is simpler/better, it restores the old logic.
> 
> Ps. submitting patches to bugzilla is a good way to have them ignored.
> 
>>From Stephen Hemminger <stephen@networkplumber.org>
> 
> Check that incoming STP packet is received on a port assigned to bridge
> before processing. It is possible to receive packet on non-bridge port
> because they are multicast.
> 
> See:
>  https://bugzilla.kernel.org/show_bug.cgi?id=64911
> 
> 
> Regression introduced by:
> commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2
> Author: Hong Zhiguo <zhiguohong@tencent.com>
> Date:   Sat Sep 14 22:42:28 2013 +0800
> 
>     bridge: fix NULL pointer deref of br_port_get_rcu
> 
> 
> Reported-by: Alexander Y. Fomichev
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> 
> --- a/net/bridge/br_stp_bpdu.c	2013-06-11 09:50:21.522919061 -0700
> +++ b/net/bridge/br_stp_bpdu.c	2013-12-05 08:46:56.090463702 -0800
> @@ -153,6 +153,9 @@ void br_stp_rcv(const struct stp_proto *
>  	if (buf[0] != 0 || buf[1] != 0 || buf[2] != 0)
>  		goto err;
>  
> +	if (!br_port_exists(dev))
> +		goto err;
> +
>  	p = br_port_get_rcu(dev);
>  	if (!p)
>  		goto err;


We alreay did some cleanup jobs before mark this dev is not a port of bridge (dev->priv_flags &= ~IFF_BRIDGE_PORT),
such as remove the fdb related to this port(br_fdb_delete_by_port).

and seems like after these cleanup jobs, before unregister this device, if new skb is received,
br_handle_local_finish will call br_fdb_update to create a new fdb whose dst points to the will-be-destroied-port.

I don't know if this will cause some problems.
seems we should also make sure port is unavailable before we do cleanup.

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[David Miller]

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.

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Stephen Hemminger]

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.

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[David Miller]

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Fri, 6 Dec 2013 13:10:28 -0800

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

I thought Jiri's response to your suggestion was adequate, that's
all.

I can revert and take your version if you want me to, just make
a formal submission and get agreement from Jiri.

Thanks.

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Vlad Yasevich]

On 12/05/2013 09:26 PM, Gao feng wrote:
> On 12/06/2013 12:55 AM, Stephen Hemminger wrote:
>> On Thu,  5 Dec 2013 16:27:37 +0100
>> Jiri Pirko <jiri@resnulli.us> 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.
>>>
>>
>>
>> I think this patch is simpler/better, it restores the old logic.
>>
>> Ps. submitting patches to bugzilla is a good way to have them ignored.
>>
>> >From Stephen Hemminger <stephen@networkplumber.org>
>>
>> Check that incoming STP packet is received on a port assigned to bridge
>> before processing. It is possible to receive packet on non-bridge port
>> because they are multicast.
>>
>> See:
>>  https://bugzilla.kernel.org/show_bug.cgi?id=64911
>>
>>
>> Regression introduced by:
>> commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2
>> Author: Hong Zhiguo <zhiguohong@tencent.com>
>> Date:   Sat Sep 14 22:42:28 2013 +0800
>>
>>     bridge: fix NULL pointer deref of br_port_get_rcu
>>
>>
>> Reported-by: Alexander Y. Fomichev
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>
>>
>> --- a/net/bridge/br_stp_bpdu.c	2013-06-11 09:50:21.522919061 -0700
>> +++ b/net/bridge/br_stp_bpdu.c	2013-12-05 08:46:56.090463702 -0800
>> @@ -153,6 +153,9 @@ void br_stp_rcv(const struct stp_proto *
>>  	if (buf[0] != 0 || buf[1] != 0 || buf[2] != 0)
>>  		goto err;
>>  
>> +	if (!br_port_exists(dev))
>> +		goto err;
>> +
>>  	p = br_port_get_rcu(dev);
>>  	if (!p)
>>  		goto err;
> 
> 
> We alreay did some cleanup jobs before mark this dev is not a port of bridge (dev->priv_flags &= ~IFF_BRIDGE_PORT),
> such as remove the fdb related to this port(br_fdb_delete_by_port).
> 
> and seems like after these cleanup jobs, before unregister this device, if new skb is received,
> br_handle_local_finish will call br_fdb_update to create a new fdb whose dst points to the will-be-destroied-port.

Not really.  We disable the port first before removing the fdb
as a result, br_handle_local_finish() will not update the fdb because
the port is in disabled state.

-vlad

> 
> I don't know if this will cause some problems.
> seems we should also make sure port is unavailable before we do cleanup.
> --
> 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
> 

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Jiri Pirko]

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.

Jiri

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Stephen Hemminger]

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>

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Jiri Pirko]

Sat, Dec 07, 2013 at 06:42:35PM CET, stephen@networkplumber.org wrote:
>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).

Sure. I will send follow-up patch addressing it. Thanks.

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

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Vlad Yasevich]

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
> 

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Jiri Pirko]

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

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Vlad Yasevich]

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

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Jiri Pirko]

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

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Michael S. Tsirkin]

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.


> +}


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?

---

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 *);

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Jiri Pirko]

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 *);

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Vlad Yasevich]

On 12/09/2013 06:58 AM, Michael S. Tsirkin 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.
> 
> 
>> +}
> 
> 
> 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?
> 
> ---
> 
> 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>

This looks very nice is a usefull API.

Acked-by: Vlad Yasevich <vyasevic@redhat.com>

however, as I mentioned to Jiri, I've been trying to understand why
Stephen's patch is insufficient and so far I can't come up with a race
scenario that would break a simple check for dev->priv_flags.

So, I've decided to look at the history that Jiri mentioned in his
commit.  In particular, I was reading
    commit b5ed54e94d324f17c97852296d61a143f01b227a
    "bridge: fix RCU races with bridge port"

that claimed that there is a race in RCU section when just checking
the priv_flags for IFF_BRIDGE_PORT flag.  Doing a little more digging
shows that at the time that commit was added, there was no call to
synchronise_net() in netdev_rx_handler_unregister().  So, at the time
of that commit there truly was a race, and the race still was not fixed
until Eric submitted
    commit 00cfec37484761a44a3b6f4675a54caa618210ae
    net: add a synchronize_net() in netdev_rx_handler_unregister()

So, I think now it is perfectly safe to simply use the construct
    if (!br_port_exists(dev))
         return;

    port = br_port_get_rcu(dev);

under rcu protection.  In fact, we are guaranteed to have a valid
bridge port in this situation due to the fact that the the flag is
is turned off before netdev_rx_handler_unregister() is called.

-vlad

-vlad

> 
> -->
> 
> 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 *);
> --
> 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
> 

Re: [patch net/stable v2] br: fix use of ->rx_handler_data in code executed on non-rx_handler path

[Jiri Pirko]

Mon, Dec 09, 2013 at 08:31:49PM CET, vyasevic@redhat.com wrote:
>On 12/09/2013 06:58 AM, Michael S. Tsirkin 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.
>> 
>> 
>>> +}
>> 
>> 
>> 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?
>> 
>> ---
>> 
>> 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>
>
>This looks very nice is a usefull API.
>
>Acked-by: Vlad Yasevich <vyasevic@redhat.com>
>
>however, as I mentioned to Jiri, I've been trying to understand why
>Stephen's patch is insufficient and so far I can't come up with a race
>scenario that would break a simple check for dev->priv_flags.
>
>So, I've decided to look at the history that Jiri mentioned in his
>commit.  In particular, I was reading
>    commit b5ed54e94d324f17c97852296d61a143f01b227a
>    "bridge: fix RCU races with bridge port"
>
>that claimed that there is a race in RCU section when just checking
>the priv_flags for IFF_BRIDGE_PORT flag.  Doing a little more digging
>shows that at the time that commit was added, there was no call to
>synchronise_net() in netdev_rx_handler_unregister().  So, at the time
>of that commit there truly was a race, and the race still was not fixed
>until Eric submitted
>    commit 00cfec37484761a44a3b6f4675a54caa618210ae
>    net: add a synchronize_net() in netdev_rx_handler_unregister()
>
>So, I think now it is perfectly safe to simply use the construct
>    if (!br_port_exists(dev))
>         return;
>
>    port = br_port_get_rcu(dev);
>
>under rcu protection.  In fact, we are guaranteed to have a valid
>bridge port in this situation due to the fact that the the flag is
>is turned off before netdev_rx_handler_unregister() is called.


You are right. The check Stephen suggested is enough. But even still,
checking against the "paired" rcu pointer (dev->rx_handler) seems nicer
here. And with Michael's generic patch, this can be done for all
rx_handler users without them taking care of it (flags, etc)
individually.


>
>-vlad
>
>-vlad
>
>> 
>> -->
>> 
>> 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 *);
>> --
>> 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
>> 
>