Re: [PATCH] protect remove_proc_entry

[Andrew Morton <akpm@osdl.org>]

Steven Rostedt <rostedt@goodmis.org> wrote:
>
> God, we should be getting rid of the stupid BKL, not add more.  But
>  seeing that this is what is used to protect that list, I guess I'll add
>  it.
> 
>  I'm also assuming that interrupt context wont use this.
> 
>  -- Steve
> 
>  Index: linux-2.6.15-rc7/fs/proc/generic.c
>  ===================================================================
>  --- linux-2.6.15-rc7.orig/fs/proc/generic.c	2005-12-30 14:19:39.000000000 -0500
>  +++ linux-2.6.15-rc7/fs/proc/generic.c	2005-12-30 17:05:56.000000000 -0500
>  @@ -693,6 +693,8 @@
>   	if (!parent && xlate_proc_name(name, &parent, &fn) != 0)
>   		goto out;
>   	len = strlen(fn);
>  +
>  +	lock_kernel();
>   	for (p = &parent->subdir; *p; p=&(*p)->next ) {
>   		if (!proc_match(len, fn, *p))
>   			continue;
>  @@ -713,6 +715,7 @@
>   		}
>   		break;
>   	}
>  +	unlock_kernel();
>   out:
>   	return;
>   }

OK, we're kind of screwed here.

Debug: sleeping function called from invalid context at include/asm/semaphore.h:105
in_atomic():1, irqs_disabled():0

Call Trace:<ffffffff8012689b>{__might_sleep+190} <ffffffff803e83ce>{lock_kernel+53}
       <ffffffff801a6db2>{remove_proc_entry+74} <ffffffff8016b295>{poison_obj+58}
       <ffffffff80134ee5>{unregister_proc_table+121} <ffffffff80134eb6>{unregister_proc_table+74}
       <ffffffff80134eb6>{unregister_proc_table+74} <ffffffff80134eb6>{unregister_proc_table+74}
       <ffffffff80134eb6>{unregister_proc_table+74} <ffffffff80134fdb>{unregister_sysctl_table+232}
       <ffffffff803a2a0e>{ip_mc_dec_group+181} <ffffffff803e802e>{_write_lock_irqsave+32}
       <ffffffff80133dc2>{local_bh_enable+114} <ffffffff803e82b3>{_write_unlock_bh+24}
       <ffffffff8039ebfe>{devinet_sysctl_unregister+31} <ffffffff8039ecc1>{inetdev_destroy+171}
       <ffffffff8039f1c1>{inet_del_ifa+509} <ffffffff8039f2dc>{inet_rtm_deladdr+268}
       <ffffffff8036efea>{rtnetlink_rcv_msg+437} <ffffffff803761c3>{netlink_run_queue+140}
       <ffffffff8036ee35>{rtnetlink_rcv_msg+0} <ffffffff8036f032>{rtnetlink_rcv+41}
       <ffffffff803758af>{netlink_data_ready+23} <ffffffff80374d77>{netlink_sendskb+41}
       <ffffffff80374ff4>{netlink_unicast+539} <ffffffff80375881>{netlink_sendmsg+667}
       <ffffffff8035bedf>{sock_sendmsg+232} <ffffffff803e8203>{_read_unlock_irq+20}
       <ffffffff80142e90>{autoremove_wake_function+0} <ffffffff80171a1e>{fget+157}
       <ffffffff80142e90>{autoremove_wake_function+0} <ffffffff80171a1e>{fget+157}
       <ffffffff8035bbea>{sockfd_lookup+18} <ffffffff8035d408>{sys_sendto+246}
       <ffffffff803e80cc>{_spin_unlock_irqrestore+27} <ffffffff80238031>{__up_write+371}
       <ffffffff8010db46>{system_call+126} 


Because CONFIG_PREEMPT_BKL makes lock_kernel do down() and some callers of
remove_proc_entry() do it from inside spinlock.

And the spinlocking variant of lock_kernel() is also pretty much illegal,
because (in this case) whatever lock networking has taken may be taken
elsewhere inside lock_kernel(), so we have an ab/ba deadlock.

Best I can think of is that we need a private spinlock for this list.