linux-kernel - Re: [rcf/patch] netpoll: Make it RT friendly

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YboSe4NfbCeoqF1j@linutronix.de>
Date:   Wed, 15 Dec 2021 17:06:19 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Mike Galbraith <efault@....de>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>,
        RT <linux-rt-users@...r.kernel.org>
Subject: Re: [rcf/patch] netpoll: Make it RT friendly

On 2021-11-19 15:41:25 [+0100], Mike Galbraith wrote:
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -252,6 +252,7 @@ static void zap_completion_queue(void)
>  		clist = sd->completion_queue;
>  		sd->completion_queue = NULL;
>  		local_irq_restore(flags);
> +		put_cpu_var(softnet_data);
> 
>  		while (clist != NULL) {
>  			struct sk_buff *skb = clist;
> @@ -263,9 +264,8 @@ static void zap_completion_queue(void)
>  				__kfree_skb(skb);
>  			}
>  		}
> -	}
> -
> -	put_cpu_var(softnet_data);
> +	} else
> +		put_cpu_var(softnet_data);
>  }

Looking at the callers of zap_completion_queue() it seems that
get_cpu_var() could be replaced this_cpu_ptr() since the pointer is
stable at this point.

>  static struct sk_buff *find_skb(struct netpoll *np, int len, int reserve)
> @@ -365,16 +366,22 @@ static netdev_tx_t __netpoll_send_skb(st
> 
>  netdev_tx_t netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
>  {
> -	unsigned long flags;
> +	unsigned long __maybe_unused flags;
>  	netdev_tx_t ret;
> 
>  	if (unlikely(!np)) {
>  		dev_kfree_skb_irq(skb);
>  		ret = NET_XMIT_DROP;
>  	} else {
> -		local_irq_save(flags);
> +		if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> +			local_irq_save(flags);
> +		else
> +			rcu_read_lock_bh();
>  		ret = __netpoll_send_skb(np, skb);
> -		local_irq_restore(flags);
> +		if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> +			local_irq_restore(flags);
> +		else
> +			rcu_read_unlock_bh();
>  	}
>  	return ret;
>  }

What is the context for netpoll_send_skb()? Why do we need to disable BH
+ RCU on RT?
If interrupts are never disabled, doesn't this break the assumption made
in netpoll_tx_running()?

queue_process() is also busted.

Sebastian