| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 02 Mar 2008 20:29:58 +0000
From: James Chapman <jchapman@...alix.com>
To: Jarek Poplawski <jarkao2@...il.com>
CC: David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH][PPPOL2TP]: Fix SMP oops in pppol2tp driver
Jarek Poplawski wrote:
> James Chapman wrote, On 02/26/2008 01:14 PM:
> ...
>> Luckily, I'm in the lab where my two borrowed servers are today so I
>> have access to their consoles. Hopefully I'll be able to find out why
>> there are hanging. Btw, they don't hang if I disable irqs around the
>> ppp_input() call.
>
> Maybe you've found the same, or there is some other reason yet, but
> IMHO this locking break around ppp_input() is wrong. Probably there
> is needed more advanced solution, but this should fix the problem if
> it really exists (isn't there possible a race e.g. between receive
> from socket and from network card?).
I tried your patch but the result is the same.
>
> Jarek P.
> ---
>
> drivers/net/pppol2tp.c | 16 ++++++++--------
> 1 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/pppol2tp.c b/drivers/net/pppol2tp.c
> index e0b072d..7c6fcb9 100644
> --- a/drivers/net/pppol2tp.c
> +++ b/drivers/net/pppol2tp.c
> @@ -363,18 +363,17 @@ out:
> spin_unlock(&session->reorder_q.lock);
> }
>
> -/* Dequeue a single skb.
> +/* Requeue a single skb.
> */
> -static void pppol2tp_recv_dequeue_skb(struct pppol2tp_session *session, struct sk_buff *skb)
> +static void pppol2tp_recv_requeue_skb(struct pppol2tp_session *session, struct sk_buff *skb)
> {
> struct pppol2tp_tunnel *tunnel = session->tunnel;
> int length = PPPOL2TP_SKB_CB(skb)->length;
> struct sock *session_sock = NULL;
>
> - /* We're about to requeue the skb, so unlink it and return resources
> + /* We're about to requeue the skb, so return resources
> * to its current owner (a socket receive buffer).
> */
> - skb_unlink(skb, &session->reorder_q);
> skb_orphan(skb);
>
> tunnel->stats.rx_packets++;
> @@ -436,14 +435,14 @@ static void pppol2tp_recv_dequeue_skb(struct pppol2tp_session *session, struct s
> static void pppol2tp_recv_dequeue(struct pppol2tp_session *session)
> {
> struct sk_buff *skb;
> - struct sk_buff *tmp;
>
> /* If the pkt at the head of the queue has the nr that we
> * expect to send up next, dequeue it and any other
> * in-sequence packets behind it.
> */
> +again:
> spin_lock(&session->reorder_q.lock);
> - skb_queue_walk_safe(&session->reorder_q, skb, tmp) {
> + skb_queue_walk(&session->reorder_q, skb) {
I think this needs the _safe() call because the list may be modified in
the loop body.
> if (time_after(jiffies, PPPOL2TP_SKB_CB(skb)->expires)) {
> session->stats.rx_seq_discards++;
> session->stats.rx_errors++;
> @@ -469,9 +468,10 @@ static void pppol2tp_recv_dequeue(struct pppol2tp_session *session)
> goto out;
> }
> }
> + __skb_unlink(skb, &session->reorder_q);
> spin_unlock(&session->reorder_q.lock);
> - pppol2tp_recv_dequeue_skb(session, skb);
> - spin_lock(&session->reorder_q.lock);
> + pppol2tp_recv_requeue_skb(session, skb);
> + goto again;
I'd prefer to take the spinlock again after the recv_dequeue_skb() call
to avoid the goto.
--
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists