netdev - Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe

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:	Mon, 18 Aug 2014 10:52:03 +0200
From:	Takashi Iwai <tiwai@...e.de>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	"Luis R. Rodriguez" <mcgrof@...e.com>,
	"Luis R. Rodriguez" <mcgrof@...not-panic.com>,
	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
	Joseph Salisbury <joseph.salisbury@...onical.com>,
	Kay Sievers <kay@...y.org>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Tim Gardner <tim.gardner@...onical.com>,
	Pierre Fersing <pierre-fersing@...rref.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Benjamin Poirier <bpoirier@...e.de>,
	Nagalakshmi Nandigama <nagalakshmi.nandigama@...gotech.com>,
	Praveen Krishnamoorthy <praveen.krishnamoorthy@...gotech.com>,
	Sreekanth Reddy <sreekanth.reddy@...gotech.com>,
	Abhijit Mahajan <abhijit.mahajan@...gotech.com>,
	Hariprasad S <hariprasad@...lsio.com>,
	Santosh Rastapur <santosh@...lsio.com>,
	MPT-FusionLinux.pdl@...gotech.com, linux-scsi@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init()	and module_long_probe_exit()

At Sun, 17 Aug 2014 20:21:38 +0200,
Oleg Nesterov wrote:
> 
> On 08/17, Luis R. Rodriguez wrote:
> >
> > In the last iteration that I have stress tested for corner cases I just
> > get_task_struct() on the init and then put_task_struct() at the exit, is that
> > fine too or are there reasons to prefer the module stuff?
> 
> I am fine either way.
> 
> I like the Takashi's idea because if sys_delete_module() is called before
> initfn() completes it will return -EBUSY and not hang in TASK_UNINTERRUPTIBLE
> state. But this is not necessarily good, so I leave this to you and Takashi.

Another merit of fiddling with module count is that the thread object
isn't referred in other than module_init.  That is, we'd need only
module_init() implementation like below (thanks to Oleg's advice):

#define module_long_probe_init(initfn)				\
	static int _long_probe_##initfn(void *arg)		\
	{							\
		module_put_and_exit(initfn());			\
		return 0;					\
	}							\
	static int __init __long_probe_##initfn(void)		\
	{							\
		struct task_struct *__init_thread =		\
			kthread_create(_long_probe_##initfn,	\
				       NULL, #initfn);		\
		if (IS_ERR(__init_thread))			\
			return PTR_ERR(__init_thread);		\
		__module_get(THIS_MODULE);			\
		wake_up_process(__init_thread);			\
		return 0;					\
	}							\
	module_init(__long_probe_##initfn)

... and module_exit() remains identical as the normal version.

But, it's really a small difference, and I don't mind much which way
to take, too.

> > +/*
> > + * Linux device drivers must strive to handle driver initialization
> > + * within less than 30 seconds,
> 
> Well, perhaps the comment should name the reason ;)
> 
> > if device probing takes longer
> > + * for whatever reason asynchronous probing of devices / loading
> > + * firmware should be used. If a driver takes longer than 30 second
> > + * on the initialization path
> 
> Or if the initialization code can't handle the errors properly (say,
> mptsas can't handle the errors caused by SIGKILL).
> 
> > + * Drivers that use this helper should be considered broken and in need
> > + * of some serious love.
> > + */
> 
> Yes.
> 
> > +#define module_long_probe_init(initfn)				\
> > +	static struct task_struct *__init_thread;		\
> > +	static int _long_probe_##initfn(void *arg)		\
> > +	{							\
> > +		return initfn();				\
> > +	}							\
> > +	static inline __init int __long_probe_##initfn(void)	\
> > +	{							\
> > +		__init_thread = kthread_create(_long_probe_##initfn,\
> > +					       NULL,		\
> > +					       #initfn);	\
> > +		if (IS_ERR(__init_thread))			\
> > +			return PTR_ERR(__init_thread);		\
> > +		/*						\
> > +		 * callback won't check kthread_should_stop()	\
> > +		 * before bailing, so we need to protect it	\
> > +		 * before running it.				\
> > +		 */						\
> > +		get_task_struct(__init_thread); 		\
> > +		wake_up_process(__init_thread);			\
> > +		return 0;					\
> > +	}							\
> > +	module_init(__long_probe_##initfn);
> > +
> > +/* To be used by modules that require module_long_probe_init() */
> > +#define module_long_probe_exit(exitfn)				\
> > +	static inline void __long_probe_##exitfn(void)		\
> > +	{							\
> > +		int err;					\
> > +		/*						\
> > +		 * exitfn() will not be run if the driver's	\
> > +		 * real probe which is run on the kthread	\
> > +		 * failed for whatever reason, this will	\
> > +		 * wait for it to end.				\
> > +		 */						\
> > +		err = kthread_stop(__init_thread);		\
> > +		if (!err)					\
> > +			exitfn();				\
> > +		put_task_struct(__init_thread);	 		\
> > +	}							\
> > +	module_exit(__long_probe_##exitfn);
> 
> Both inline's look misleading, gcc will generate the code out-of-line
> anyway. But this is cosmetic. And for cosmetic reasons, since the 1st
> macro uses __init, the 2nd one should probably use __exit.

Yes, and it'd be better to mention not to mark initfn with __init
prefix.  (Meanwhile exitfn can be with __exit prefix.)


thanks,

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