From: Daniel Borkmann <dborkman@redhat.com>
To: Salam Noureddine <noureddine@aristanetworks.com>
Cc: "David S. Miller" <davem@davemloft.net>,
Willem de Bruijn <willemb@google.com>, Phil Sutter <phil@nwl.cc>,
Eric Dumazet <edumazet@google.com>,
netdev@vger.kernel.org, Ben Greear <greearb@candelatech.com>
Subject: Re: [PATCH 1/1] Revert "af-packet: Use existing netdev reference for bound sockets"
Date: Mon, 18 Nov 2013 10:00:53 +0100 [thread overview]
Message-ID: <5289D745.5020200@redhat.com> (raw)
In-Reply-To: <1384760425-37658-1-git-send-email-noureddine@aristanetworks.com>
On 11/18/2013 08:40 AM, Salam Noureddine wrote:
> This reverts commit 827d978037d7d0bf0860481948c6d26ead10042f
> ("af-packet: Use existing netdev reference for bound sockets")
>
> The patch introduced a race condition between packet_snd and
> packet_notifier when a net_device is being unregistered. In the case of
> a bound socket, packet_notifier can drop the last reference to the
> net_device and packet_snd might end up sending a packet over a freed
> net_device.
So there's no other workaround possible like e.g. setting a flag in
struct packet_sock so that in case our netdevice goes down, we just
set the flag and if set, we return with -ENXIO in send path?
Reverting this would decrease performance for everyone as we would
then do the lookup every time we send a packet again.
> Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>
> ---
> net/packet/af_packet.c | 26 +++++++++++---------------
> 1 files changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 2e8286b..34fe9eb 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2057,8 +2057,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> struct sk_buff *skb;
> struct net_device *dev;
> __be16 proto;
> - bool need_rls_dev = false;
> - int err, reserve = 0;
> + int ifindex, err, reserve = 0;
> void *ph;
> struct sockaddr_ll *saddr = (struct sockaddr_ll *)msg->msg_name;
> int tp_len, size_max;
> @@ -2070,7 +2069,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> mutex_lock(&po->pg_vec_lock);
>
> if (saddr == NULL) {
> - dev = po->prot_hook.dev;
> + ifindex = po->ifindex;
> proto = po->num;
> addr = NULL;
> } else {
> @@ -2081,12 +2080,12 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> + offsetof(struct sockaddr_ll,
> sll_addr)))
> goto out;
> + ifindex = saddr->sll_ifindex;
> proto = saddr->sll_protocol;
> addr = saddr->sll_addr;
> - dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
> - need_rls_dev = true;
> }
>
> + dev = dev_get_by_index(sock_net(&po->sk), ifindex);
> err = -ENXIO;
> if (unlikely(dev == NULL))
> goto out;
> @@ -2173,8 +2172,7 @@ out_status:
> __packet_set_status(po, ph, status);
> kfree_skb(skb);
> out_put:
> - if (need_rls_dev)
> - dev_put(dev);
> + dev_put(dev);
> out:
> mutex_unlock(&po->pg_vec_lock);
> return err;
> @@ -2212,9 +2210,8 @@ static int packet_snd(struct socket *sock,
> struct sk_buff *skb;
> struct net_device *dev;
> __be16 proto;
> - bool need_rls_dev = false;
> unsigned char *addr;
> - int err, reserve = 0;
> + int ifindex, err, reserve = 0;
> struct virtio_net_hdr vnet_hdr = { 0 };
> int offset = 0;
> int vnet_hdr_len;
> @@ -2228,7 +2225,7 @@ static int packet_snd(struct socket *sock,
> */
>
> if (saddr == NULL) {
> - dev = po->prot_hook.dev;
> + ifindex = po->ifindex;
> proto = po->num;
> addr = NULL;
> } else {
> @@ -2237,12 +2234,12 @@ static int packet_snd(struct socket *sock,
> goto out;
> if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
> goto out;
> + ifindex = saddr->sll_ifindex;
> proto = saddr->sll_protocol;
> addr = saddr->sll_addr;
> - dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
> - need_rls_dev = true;
> }
>
> + dev = dev_get_by_index(sock_net(sk), ifindex);
> err = -ENXIO;
> if (dev == NULL)
> goto out_unlock;
> @@ -2386,15 +2383,14 @@ static int packet_snd(struct socket *sock,
> if (err > 0 && (err = net_xmit_errno(err)) != 0)
> goto out_unlock;
>
> - if (need_rls_dev)
> - dev_put(dev);
> + dev_put(dev);
>
> return len;
>
> out_free:
> kfree_skb(skb);
> out_unlock:
> - if (dev && need_rls_dev)
> + if (dev)
> dev_put(dev);
> out:
> return err;
>
next prev parent reply other threads:[~2013-11-18 9:01 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-18 7:40 [PATCH 1/1] Revert "af-packet: Use existing netdev reference for bound sockets" Salam Noureddine
2013-11-18 9:00 ` Daniel Borkmann [this message]
2013-11-18 18:03 ` David Miller
2013-11-18 23:39 ` Daniel Borkmann
2013-11-19 0:17 ` Salam Noureddine
2013-11-19 1:26 ` David Miller
2013-11-19 1:30 ` David Miller
-- strict thread matches above, loose matches on Subject: below --
2013-11-18 5:32 Salam Noureddine
2013-11-18 7:42 ` Salam Noureddine
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5289D745.5020200@redhat.com \
--to=dborkman@redhat.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=greearb@candelatech.com \
--cc=netdev@vger.kernel.org \
--cc=noureddine@aristanetworks.com \
--cc=phil@nwl.cc \
--cc=willemb@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).