BUG: using smp_processor_id() in preemptible [00000000] code: avahi-daemon: caller is netif_rx

On Thu, Apr 15, 2010 at 3:57 PM, David Miller <davem@davemloft.net> wrote:
>
> Why?  If we are in an interrupt (either soft or hard) then
> smp_processor_id() is stable, and valid.
>
> Changli, I find it very frustrating to communicate with you, you are
> very terse in your descriptions and analysis and you make many simple
> errors that would be avoided if you spent more time thinking about
> things before sending your emails. :-/
>
> Instead of just showing some pseudo patch, state WHY it is needed.
> Talk about the execution state of environment and what rules or other
> things are being violated which must be corrected.
>

Sorry. English isn't my native language, so sometimes I can't describe
myself clearly.

I think the following patch from Eric should be applied instead.

As you know  "netif_rx() must be invoked from a hardware or software
interrupt, which implies preemption disabled.", obviously
ip_dev_loopback_xmit() doesn't obey this rule, so the crash isn't the
fault of net_rx(). If there are other users, who don't obey this rule,
they should be fixed too.

For this patch:

-       cpu = smp_processor_id();
+       ret = enqueue_to_backlog(skb, get_cpu());
+       put_cpu();

You said: " If we are in an interrupt (either soft or hard) then
smp_processor_id() is stable, and valid.". so we don't need to call
get_cpu() instead of smp_processor_id(). get_cpu() brings no good but
additional cost preempt_disable().

From: Changli Gao <xiaosuo@gmail.com>
Date: Thu, 15 Apr 2010 16:27:19 +0800

> I think the following patch from Eric should be applied instead.
> 
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index c65f18e..d1bcc9f 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -120,7 +120,7 @@ static int ip_dev_loopback_xmit(struct sk_buff *newskb)
>        newskb->pkt_type = PACKET_LOOPBACK;
>        newskb->ip_summed = CHECKSUM_UNNECESSARY;
>        WARN_ON(!skb_dst(newskb));
> -       netif_rx(newskb);
> +       netif_rx_ni(newskb);
>        return 0;
>  }

Yes, this looks more reasonable.  Eric if you agree please (re-)submit
this formally, I must have missed this somehow, sorry.

And this is a bug fix in any kernel, not just one's that have RPS
patches applied.

If we are not called from some interrupt context, there is no sure
trigger to make sure software interrupts will be executed after the
packet is queued locally.  netif_rx_ni() makes sure that any pending
software interrupts will run in such cases.
--
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

Le jeudi 15 avril 2010 à 01:33 -0700, David Miller a écrit :

> Yes, this looks more reasonable.  Eric if you agree please (re-)submit
> this formally, I must have missed this somehow, sorry.
> 
> And this is a bug fix in any kernel, not just one's that have RPS
> patches applied.
> 
> If we are not called from some interrupt context, there is no sure
> trigger to make sure software interrupts will be executed after the
> packet is queued locally.  netif_rx_ni() makes sure that any pending
> software interrupts will run in such cases.

Our mails crossed ;)

Yes I think it's more reasonable to fix it like this, I'll submit a
patch after fully testing 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