| Message ID | 7125.1540602020@famine |
|---|---|
| State | New |
| Headers | show |
| Series | [TRUSTY,SRU] UBUNTU: SAUCE: (no-up) net/packet: fix erroneous dev_add_pack usage in fanout | expand |
On 2018-10-26 18:00:20 , Jay Vosburgh wrote: > BugLink: http://bugs.launchpad.net/bugs/1800254 > > Due to changes added as part of c108ac876c02 ("packet: hold bind lock when > rebinding to fanout hook"), it is possible for fanout_add to add a > packet_type handler via dev_add_pack and then kfree the memory backing the > packet_type. This corrupts the ptype_all list, causing the system to > panic when network packet processing next traverses ptype_all. The > erroneous path is taken when a PACKET_FANOUT setsockopt is performed on a > packet socket that is bound to an interface that is administratively down. > > This is not due to any flaw of c108ac876c02, but rather than the packet > socket code base differs subtly in 3.13 as compared to 4.4. > > The remedy for this is to include additional changes in the management > of the dev_add_pack calls from 4.4. This moves the dev_add_pack and > dev_remove_pack calls from fanout_add and _release into __fanout_link > and _unlink. > > These changes originate in 2bd624b4611f ("packet: Do not call > fanout_release from atomic contexts"). We do not include that patch in > its entirety as it has other dependencies, and this is the minimal > change set to resolve the issue. > > Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> > > --- > net/packet/af_packet.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index c0230c7458df..fa02443df232 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -1267,6 +1267,8 @@ static void __fanout_link(struct sock *sk, struct packet_sock *po) > f->arr[f->num_members] = sk; > smp_wmb(); > f->num_members++; > + if (f->num_members == 1) > + dev_add_pack(&f->prot_hook); > spin_unlock(&f->lock); > } > > @@ -1283,6 +1285,8 @@ static void __fanout_unlink(struct sock *sk, struct packet_sock *po) > BUG_ON(i >= f->num_members); > f->arr[i] = f->arr[f->num_members - 1]; > f->num_members--; > + if (f->num_members == 0) > + __dev_remove_pack(&f->prot_hook); Should the above line be a call to dev_remove_pack() instead of __dev_remove_pack() ? > spin_unlock(&f->lock); > } > > @@ -1350,7 +1354,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags) > match->prot_hook.func = packet_rcv_fanout; > match->prot_hook.af_packet_priv = match; > match->prot_hook.id_match = match_fanout_group; > - dev_add_pack(&match->prot_hook); > list_add(&match->list, &fanout_list); > } > err = -EINVAL; > @@ -1393,7 +1396,6 @@ static void fanout_release(struct sock *sk) > > if (atomic_dec_and_test(&f->sk_ref)) { > list_del(&f->list); > - dev_remove_pack(&f->prot_hook); > kfree(f); > } > } > -- > 2.7.4 > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Khaled Elmously <khalid.elmously@canonical.com> wrote: >On 2018-10-26 18:00:20 , Jay Vosburgh wrote: >> BugLink: http://bugs.launchpad.net/bugs/1800254 >> >> Due to changes added as part of c108ac876c02 ("packet: hold bind lock when >> rebinding to fanout hook"), it is possible for fanout_add to add a >> packet_type handler via dev_add_pack and then kfree the memory backing the >> packet_type. This corrupts the ptype_all list, causing the system to >> panic when network packet processing next traverses ptype_all. The >> erroneous path is taken when a PACKET_FANOUT setsockopt is performed on a >> packet socket that is bound to an interface that is administratively down. >> >> This is not due to any flaw of c108ac876c02, but rather than the packet >> socket code base differs subtly in 3.13 as compared to 4.4. >> >> The remedy for this is to include additional changes in the management >> of the dev_add_pack calls from 4.4. This moves the dev_add_pack and >> dev_remove_pack calls from fanout_add and _release into __fanout_link >> and _unlink. >> >> These changes originate in 2bd624b4611f ("packet: Do not call >> fanout_release from atomic contexts"). We do not include that patch in >> its entirety as it has other dependencies, and this is the minimal >> change set to resolve the issue. >> >> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> >> >> --- >> net/packet/af_packet.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c >> index c0230c7458df..fa02443df232 100644 >> --- a/net/packet/af_packet.c >> +++ b/net/packet/af_packet.c >> @@ -1267,6 +1267,8 @@ static void __fanout_link(struct sock *sk, struct packet_sock *po) >> f->arr[f->num_members] = sk; >> smp_wmb(); >> f->num_members++; >> + if (f->num_members == 1) >> + dev_add_pack(&f->prot_hook); >> spin_unlock(&f->lock); >> } >> >> @@ -1283,6 +1285,8 @@ static void __fanout_unlink(struct sock *sk, struct packet_sock *po) >> BUG_ON(i >= f->num_members); >> f->arr[i] = f->arr[f->num_members - 1]; >> f->num_members--; >> + if (f->num_members == 0) >> + __dev_remove_pack(&f->prot_hook); > >Should the above line be a call to dev_remove_pack() instead of __dev_remove_pack() ? No, it cannot call dev_remove_pack because a lock is held here and dev_remove_pack will call synchronize_net(), which may sleep. Call paths into __fanout_unlink will manage the synchronize_net() or equivalent separately, e.g., packet_set_ring calls __unregister_prot_hook with sync==false, but it is holding a lock, and calls synchronize_net() immediately after releasing the lock. The tricky one is packet_notifier, which doesn't call synchronize_net() itself, nor does its caller. packet_notifier is called with RTNL held, and rtnl_unlock has some RCU implications, but doesn't seem to guarantee a full RCU grace period. So, I'm not entirely sure on this particular case. However, the upstream logic is unchanged for this case, so either I'm missing something and this is fine or there's a heretofore undiscovered bug in the upstream code. -J >> spin_unlock(&f->lock); >> } >> >> @@ -1350,7 +1354,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags) >> match->prot_hook.func = packet_rcv_fanout; >> match->prot_hook.af_packet_priv = match; >> match->prot_hook.id_match = match_fanout_group; >> - dev_add_pack(&match->prot_hook); >> list_add(&match->list, &fanout_list); >> } >> err = -EINVAL; >> @@ -1393,7 +1396,6 @@ static void fanout_release(struct sock *sk) >> >> if (atomic_dec_and_test(&f->sk_ref)) { >> list_del(&f->list); >> - dev_remove_pack(&f->prot_hook); >> kfree(f); >> } >> } >> -- >> 2.7.4 --- -Jay Vosburgh, jay.vosburgh@canonical.com
On 27.10.18 03:00, Jay Vosburgh wrote: > BugLink: http://bugs.launchpad.net/bugs/1800254 > > Due to changes added as part of c108ac876c02 ("packet: hold bind lock when > rebinding to fanout hook"), it is possible for fanout_add to add a > packet_type handler via dev_add_pack and then kfree the memory backing the > packet_type. This corrupts the ptype_all list, causing the system to > panic when network packet processing next traverses ptype_all. The > erroneous path is taken when a PACKET_FANOUT setsockopt is performed on a > packet socket that is bound to an interface that is administratively down. > > This is not due to any flaw of c108ac876c02, but rather than the packet > socket code base differs subtly in 3.13 as compared to 4.4. > > The remedy for this is to include additional changes in the management > of the dev_add_pack calls from 4.4. This moves the dev_add_pack and > dev_remove_pack calls from fanout_add and _release into __fanout_link > and _unlink. > > These changes originate in 2bd624b4611f ("packet: Do not call > fanout_release from atomic contexts"). We do not include that patch in > its entirety as it has other dependencies, and this is the minimal > change set to resolve the issue. > > Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > > --- Limited scope and tested. -Stefan > net/packet/af_packet.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index c0230c7458df..fa02443df232 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -1267,6 +1267,8 @@ static void __fanout_link(struct sock *sk, struct packet_sock *po) > f->arr[f->num_members] = sk; > smp_wmb(); > f->num_members++; > + if (f->num_members == 1) > + dev_add_pack(&f->prot_hook); > spin_unlock(&f->lock); > } > > @@ -1283,6 +1285,8 @@ static void __fanout_unlink(struct sock *sk, struct packet_sock *po) > BUG_ON(i >= f->num_members); > f->arr[i] = f->arr[f->num_members - 1]; > f->num_members--; > + if (f->num_members == 0) > + __dev_remove_pack(&f->prot_hook); > spin_unlock(&f->lock); > } > > @@ -1350,7 +1354,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags) > match->prot_hook.func = packet_rcv_fanout; > match->prot_hook.af_packet_priv = match; > match->prot_hook.id_match = match_fanout_group; > - dev_add_pack(&match->prot_hook); > list_add(&match->list, &fanout_list); > } > err = -EINVAL; > @@ -1393,7 +1396,6 @@ static void fanout_release(struct sock *sk) > > if (atomic_dec_and_test(&f->sk_ref)) { > list_del(&f->list); > - dev_remove_pack(&f->prot_hook); > kfree(f); > } > } >
On 10/27/18 03:00, Jay Vosburgh wrote: > BugLink: http://bugs.launchpad.net/bugs/1800254 > > Due to changes added as part of c108ac876c02 ("packet: hold bind lock when > rebinding to fanout hook"), it is possible for fanout_add to add a > packet_type handler via dev_add_pack and then kfree the memory backing the > packet_type. This corrupts the ptype_all list, causing the system to > panic when network packet processing next traverses ptype_all. The > erroneous path is taken when a PACKET_FANOUT setsockopt is performed on a > packet socket that is bound to an interface that is administratively down. > > This is not due to any flaw of c108ac876c02, but rather than the packet > socket code base differs subtly in 3.13 as compared to 4.4. > > The remedy for this is to include additional changes in the management > of the dev_add_pack calls from 4.4. This moves the dev_add_pack and > dev_remove_pack calls from fanout_add and _release into __fanout_link > and _unlink. > > These changes originate in 2bd624b4611f ("packet: Do not call > fanout_release from atomic contexts"). We do not include that patch in > its entirety as it has other dependencies, and this is the minimal > change set to resolve the issue. > > Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> Thanks for the additional explanation on the other email! > > --- > net/packet/af_packet.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index c0230c7458df..fa02443df232 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -1267,6 +1267,8 @@ static void __fanout_link(struct sock *sk, struct packet_sock *po) > f->arr[f->num_members] = sk; > smp_wmb(); > f->num_members++; > + if (f->num_members == 1) > + dev_add_pack(&f->prot_hook); > spin_unlock(&f->lock); > } > > @@ -1283,6 +1285,8 @@ static void __fanout_unlink(struct sock *sk, struct packet_sock *po) > BUG_ON(i >= f->num_members); > f->arr[i] = f->arr[f->num_members - 1]; > f->num_members--; > + if (f->num_members == 0) > + __dev_remove_pack(&f->prot_hook); > spin_unlock(&f->lock); > } > > @@ -1350,7 +1354,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags) > match->prot_hook.func = packet_rcv_fanout; > match->prot_hook.af_packet_priv = match; > match->prot_hook.id_match = match_fanout_group; > - dev_add_pack(&match->prot_hook); > list_add(&match->list, &fanout_list); > } > err = -EINVAL; > @@ -1393,7 +1396,6 @@ static void fanout_release(struct sock *sk) > > if (atomic_dec_and_test(&f->sk_ref)) { > list_del(&f->list); > - dev_remove_pack(&f->prot_hook); > kfree(f); > } > } >
Applied to trusty/master-next. Thanks. -Stefan
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index c0230c7458df..fa02443df232 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -1267,6 +1267,8 @@ static void __fanout_link(struct sock *sk, struct packet_sock *po) f->arr[f->num_members] = sk; smp_wmb(); f->num_members++; + if (f->num_members == 1) + dev_add_pack(&f->prot_hook); spin_unlock(&f->lock); } @@ -1283,6 +1285,8 @@ static void __fanout_unlink(struct sock *sk, struct packet_sock *po) BUG_ON(i >= f->num_members); f->arr[i] = f->arr[f->num_members - 1]; f->num_members--; + if (f->num_members == 0) + __dev_remove_pack(&f->prot_hook); spin_unlock(&f->lock); } @@ -1350,7 +1354,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags) match->prot_hook.func = packet_rcv_fanout; match->prot_hook.af_packet_priv = match; match->prot_hook.id_match = match_fanout_group; - dev_add_pack(&match->prot_hook); list_add(&match->list, &fanout_list); } err = -EINVAL; @@ -1393,7 +1396,6 @@ static void fanout_release(struct sock *sk) if (atomic_dec_and_test(&f->sk_ref)) { list_del(&f->list); - dev_remove_pack(&f->prot_hook); kfree(f); } }
BugLink: http://bugs.launchpad.net/bugs/1800254 Due to changes added as part of c108ac876c02 ("packet: hold bind lock when rebinding to fanout hook"), it is possible for fanout_add to add a packet_type handler via dev_add_pack and then kfree the memory backing the packet_type. This corrupts the ptype_all list, causing the system to panic when network packet processing next traverses ptype_all. The erroneous path is taken when a PACKET_FANOUT setsockopt is performed on a packet socket that is bound to an interface that is administratively down. This is not due to any flaw of c108ac876c02, but rather than the packet socket code base differs subtly in 3.13 as compared to 4.4. The remedy for this is to include additional changes in the management of the dev_add_pack calls from 4.4. This moves the dev_add_pack and dev_remove_pack calls from fanout_add and _release into __fanout_link and _unlink. These changes originate in 2bd624b4611f ("packet: Do not call fanout_release from atomic contexts"). We do not include that patch in its entirety as it has other dependencies, and this is the minimal change set to resolve the issue. Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> --- net/packet/af_packet.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)