| Message ID | 1379001428-2727-1-git-send-email-zhiguohong@tencent.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
You're right, Vlad. One thing is missing in Eric's fix, NULL dereference is still possible in br_handle_local_finish. Above is the new version of fix. On Thu, Sep 12, 2013 at 11:57 PM, Hong Zhiguo <honkiko@gmail.com> wrote: > not check IFF_BRIDGE_PORT within br_handle_frame > > Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com> > --- > net/bridge/br_input.c | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index a2fd37e..da4714a 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -60,7 +60,7 @@ static int br_pass_frame_up(struct sk_buff *skb) > int br_handle_frame_finish(struct sk_buff *skb) > { > const unsigned char *dest = eth_hdr(skb)->h_dest; > - struct net_bridge_port *p = br_port_get_rcu(skb->dev); > + enet_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data); > struct net_bridge *br; > struct net_bridge_fdb_entry *dst; > struct net_bridge_mdb_entry *mdst; > @@ -143,7 +143,7 @@ drop: > /* note: already called with rcu_read_lock */ > static int br_handle_local_finish(struct sk_buff *skb) > { > - struct net_bridge_port *p = br_port_get_rcu(skb->dev); > + struct net_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data); > u16 vid = 0; > > br_vlan_get_tag(skb, &vid); > @@ -173,7 +173,7 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb) > if (!skb) > return RX_HANDLER_CONSUMED; > > - p = br_port_get_rcu(skb->dev); > + p = rcu_dereference(skb->dev->rx_handler_data); > > if (unlikely(is_link_local_ether_addr(dest))) { > /* > -- > 1.7.0.4 >
On Fri, 2013-09-13 at 00:06 +0800, Hong zhi guo wrote: > You're right, Vlad. > One thing is missing in Eric's fix, NULL dereference is still possible > in br_handle_local_finish. Above is the new version of fix. Hey, it was not a 'fix', but a comment on your patch and bridge defensive programming. -- 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 Thu, 2013-09-12 at 23:57 +0800, Hong Zhiguo wrote: > not check IFF_BRIDGE_PORT within br_handle_frame > > Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com> > --- No need to hurry. Take the time to write an extensive changelog, and test the patch ! Thanks -- 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
sorry, Eric, maybe I'm using wrong words. Thanks for your review and help. So you both prefer not testing IFF_BRIDGE_PORT. Let's take the new fix. On Fri, Sep 13, 2013 at 12:11 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Fri, 2013-09-13 at 00:06 +0800, Hong zhi guo wrote: >> You're right, Vlad. >> One thing is missing in Eric's fix, NULL dereference is still possible >> in br_handle_local_finish. Above is the new version of fix. > > Hey, it was not a 'fix', but a comment on your patch and bridge > defensive programming. > > >
On Fri, 2013-09-13 at 00:18 +0800, Hong zhi guo wrote: > sorry, Eric, maybe I'm using wrong words. Thanks for your review and help. > So you both prefer not testing IFF_BRIDGE_PORT. Let's take the new fix. Please do not top post on netdev. Thanks -- 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
From: Hong Zhiguo <honkiko@gmail.com> Date: Thu, 12 Sep 2013 23:57:08 +0800 > @@ -60,7 +60,7 @@ static int br_pass_frame_up(struct sk_buff *skb) > int br_handle_frame_finish(struct sk_buff *skb) > { > const unsigned char *dest = eth_hdr(skb)->h_dest; > - struct net_bridge_port *p = br_port_get_rcu(skb->dev); > + enet_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data); You did not even compile test this patch. This is a good way to make people not want to review your work at all, because if you don't care enough to even compile test your patch why should other people care enough to look at it? -- 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
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index a2fd37e..da4714a 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -60,7 +60,7 @@ static int br_pass_frame_up(struct sk_buff *skb) int br_handle_frame_finish(struct sk_buff *skb) { const unsigned char *dest = eth_hdr(skb)->h_dest; - struct net_bridge_port *p = br_port_get_rcu(skb->dev); + enet_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data); struct net_bridge *br; struct net_bridge_fdb_entry *dst; struct net_bridge_mdb_entry *mdst; @@ -143,7 +143,7 @@ drop: /* note: already called with rcu_read_lock */ static int br_handle_local_finish(struct sk_buff *skb) { - struct net_bridge_port *p = br_port_get_rcu(skb->dev); + struct net_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data); u16 vid = 0; br_vlan_get_tag(skb, &vid); @@ -173,7 +173,7 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb) if (!skb) return RX_HANDLER_CONSUMED; - p = br_port_get_rcu(skb->dev); + p = rcu_dereference(skb->dev->rx_handler_data); if (unlikely(is_link_local_ether_addr(dest))) { /*
not check IFF_BRIDGE_PORT within br_handle_frame Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com> --- net/bridge/br_input.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)