| Message ID | 1392552062-17927-1-git-send-email-linus.luessing@web.de |
|---|---|
| State | Accepted, archived |
| Commit | 65d8217193427026169c48112c561c5ca4d1bd18 |
| Headers | show |
On 16/02/14 13:01, Linus Lüssing wrote: > @@ -371,12 +372,13 @@ static int __batadv_interface_tx(struct sk_buff *skb, \ > > #ifndef netdev_master_upper_dev_get_rcu > #define netdev_master_upper_dev_get_rcu(dev) \ > - NULL; \ > + upper; \ > if (dev->priv_flags & IFF_BRIDGE_PORT) { \ > rcu_read_unlock(); \ > dev_hold(dev); \ > return dev; \ > - } > + } else \ > + dev = NULL; > Following your patch the code in multicast.c will become: 172 do { 173 upper = upper; if (dev->priv_flags & IFF_BRIDGE_PORT) { rcu_read_unlock(); dev_hold(dev); return dev; } else dev = NULL; 174 } while (upper && !(upper->priv_flags & IFF_EBRIDGE)); am I wrong or this is going to break the while? I think there is a missing '}'. What about a simplified version like this: #define netdev_master_upper_dev_get_rcu(dev) \ - NULL; \ - if (dev->priv_flags & IFF_BRIDGE_PORT) { \ + ({if (dev->priv_flags & IFF_BRIDGE_PORT) { \ rcu_read_unlock(); \ dev_hold(dev); \ return dev; \ - } + }\ + NULL;}) Cheers,
On 16/02/14 14:39, Antonio Quartulli wrote: > On 16/02/14 13:01, Linus Lüssing wrote: >> @@ -371,12 +372,13 @@ static int __batadv_interface_tx(struct sk_buff *skb, \ >> >> #ifndef netdev_master_upper_dev_get_rcu >> #define netdev_master_upper_dev_get_rcu(dev) \ >> - NULL; \ >> + upper; \ >> if (dev->priv_flags & IFF_BRIDGE_PORT) { \ >> rcu_read_unlock(); \ >> dev_hold(dev); \ >> return dev; \ >> - } >> + } else \ >> + dev = NULL; >> > > Following your patch the code in multicast.c will become: > > 172 do { > 173 upper = upper; > if (dev->priv_flags & IFF_BRIDGE_PORT) { > rcu_read_unlock(); > dev_hold(dev); > return dev; > } else > dev = NULL; > 174 } while (upper && !(upper->priv_flags & IFF_EBRIDGE)); > > am I wrong or this is going to break the while? I think there is a > missing '}'. > I was wrong. I just dreamt of a '{' after "else". Forget about this comment. Cheers,
On Sunday 16 February 2014 13:01:02 Linus Lüssing wrote: > The compat code of the new multicast patchset leads to null pointer > derefernces for kernels 3.9 in netdev_master_upper_dev_get_rcu(). This > is because the initially NULL is assigned to upper, which is equal to > dev. dev is dereferenced one line later, though, leading to a crash. > > Fixing this by assigning NULL only when we are sure that the according > pointer is not going to be dereferenced anymore. > > Introduced by: 532cadf26cfbb1099ef31fae9ccafcbbfc37b9b5 > ("batman-adv: Multicast Listener Announcements via Translation Table") > > Reported-by: Marek Lindner <mareklindner@neomailbox.ch> > Signed-off-by: Linus Lüssing <linus.luessing@web.de> > --- > compat.h | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) Applied in revision 65d8217. Thanks, Marek
diff --git a/compat.h b/compat.h index 7a3d235..7beba36 100644 --- a/compat.h +++ b/compat.h @@ -162,12 +162,13 @@ static inline int batadv_param_set_copystring(const char *val, #define NET_ADDR_RANDOM 0 #define netdev_master_upper_dev_get_rcu(dev) \ - NULL; \ + upper; \ if (dev->br_port ? 1 : 0) { \ rcu_read_unlock(); \ dev_hold(dev); \ return dev; \ - } + } else \ + dev = NULL; #endif /* < KERNEL_VERSION(2, 6, 36) */ @@ -371,12 +372,13 @@ static int __batadv_interface_tx(struct sk_buff *skb, \ #ifndef netdev_master_upper_dev_get_rcu #define netdev_master_upper_dev_get_rcu(dev) \ - NULL; \ + upper; \ if (dev->priv_flags & IFF_BRIDGE_PORT) { \ rcu_read_unlock(); \ dev_hold(dev); \ return dev; \ - } + } else \ + dev = NULL; #endif /* netdev_master_upper_dev_get_rcu */
The compat code of the new multicast patchset leads to null pointer derefernces for kernels 3.9 in netdev_master_upper_dev_get_rcu(). This is because the initially NULL is assigned to upper, which is equal to dev. dev is dereferenced one line later, though, leading to a crash. Fixing this by assigning NULL only when we are sure that the according pointer is not going to be dereferenced anymore. Introduced by: 532cadf26cfbb1099ef31fae9ccafcbbfc37b9b5 ("batman-adv: Multicast Listener Announcements via Translation Table") Reported-by: Marek Lindner <mareklindner@neomailbox.ch> Signed-off-by: Linus Lüssing <linus.luessing@web.de> --- compat.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)