netdev - Re: [PATCH 1/1] netstamp_needed shouldn't be jump_label

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]

Date:	Tue, 29 Nov 2011 10:31:33 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	igorm@....rs
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH 1/1] netstamp_needed shouldn't be jump_label_key

Le mardi 29 novembre 2011 à 10:03 +0100, "Igor Maravić" a écrit :
> >
> > Could you test following patch ?
> >
> 
> I've just put it to compile. I'l let you know soon as it finishes compiling.
> 
> I have one proposition for your patch. To put WARN_ON before in ifdef endif,
> because we don't needed if we don't have HAVE_JUMP_LABEL, and also to check it
> before using jump_label_dec.
> 
> Also I have few questions :)
> 
> First - why can't we use spin_lock on jump_label?
> 
> I know that You said that is because we are using mutex_lock in
> arch_jump_label_transform() or text_poke_smp(), but I don't see why couldn't
> we use mutex_lock inside spin_lock.
> 


Try it, you'll see how bad it is.

> To me using spin_lock_irqsave and spin_lock_irqrestore does sound like as most logical solution.
> With that lock, we would be sure that our mutex_lock in arch_jump_label_transform() or text_poke_smp()
> isn't going to be interrupted.
> Please correct if I'm wrong.
> 
> Also I saw that you put calling of net_enable_timestamp outside of spin_lock_bh
> Why?
> 

Because its the same problem. We want to be allowed to sleep.

> BR
> Igor
> 
> [PATCH 1/1] jump_label warning if_interrupt
> 
> Move warning on the top of function net_enable_timestamp,
> so we would be also warn if we are going to jump_label_dec in interrupt
> 
> Signed-off-by: Igor Maravic <igorm@....rs>
> 
> :100644 100644 45eab03... ef23cf7... M	net/core/dev.c
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 45eab03..ef23cf7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1453,6 +1453,7 @@ void net_enable_timestamp(void)
>  {
>  #ifdef HAVE_JUMP_LABEL
>  	int deferred = atomic_xchg(&netstamp_needed_deferred, 0);
> +	WARN_ON(in_interrupt());
> 
>  	if (deferred) {
>  		while (--deferred)
> @@ -1460,7 +1461,6 @@ void net_enable_timestamp(void)
>  		return;
>  	}
>  #endif
> -	WARN_ON(in_interrupt());
>  	jump_label_inc(&netstamp_needed);
>  }
>  EXPORT_SYMBOL(net_enable_timestamp);

Its better to have coverage of the test, even on machines with no
HAVE_JUMP_LABEL.

So move the test before the 
#ifdef HAVE_JUMP_LABEL


--
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