diff mbox

[v3,1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

Message ID 1407882507-325-2-git-send-email-mcgrof@do-not-panic.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Luis R. Rodriguez Aug. 12, 2014, 10:28 p.m. UTC
From: "Luis R. Rodriguez" <mcgrof@suse.com>

Tetsuo bisected and found that commit 786235ee "kthread: make
kthread_create() killable" modified kthread_create() to bail as
soon as SIGKILL is received. This is causing some issues with
some drivers and at times boot. Joseph then found that failures
occur as the systemd-udevd process sends SIGKILL to modprobe if
probe on a driver takes over 30 seconds. When this happens probe
will fail on any driver, its why booting on some system will fail
if the driver happens to be a storage related driver. Some folks
have suggested fixing this by modifying kthread_create() to not
leave upon SIGKILL [3], upon review Oleg rejected this change and
the discussion was punted out to systemd to see if the default
timeout could be increased from 30 seconds to 120. The opinion of
the systemd maintainers is that the driver's behavior should
be fixed [4]. Linus seems to agree [5], however more recently even
networking drivers have been reported to fail on probe since just
writing the firmware to a device and kicking it can take easy over
60 seconds [6]. Benjamim was able to trace the issues recently
reported on cxgb4 down to the same systemd-udevd 30 second timeout [6].

This is an alternative solution which enables drivers that are
known to take long to use kthread_run(), this avoids the 30 second
timeout and lets us annotate drivers with long init sequences that
need some love.

[0] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705
[1] https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248
[2] http://lists.freedesktop.org/archives/systemd-devel/2014-March/018006.html
[3] http://thread.gmane.org/gmane.linux.ubuntu.devel.kernel.general/39123
[4] http://article.gmane.org/gmane.comp.sysutils.systemd.devel/17860
[5] http://article.gmane.org/gmane.linux.kernel/1671333
[6] https://bugzilla.novell.com/show_bug.cgi?id=877622

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <joseph.salisbury@canonical.com>
Cc: Kay Sievers <kay@vrfy.org>
Cc: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Cc: Tim Gardner <tim.gardner@canonical.com>
Cc: Pierre Fersing <pierre-fersing@pierref.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Poirier <bpoirier@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Nagalakshmi Nandigama <nagalakshmi.nandigama@avagotech.com>
Cc: Praveen Krishnamoorthy <praveen.krishnamoorthy@avagotech.com>
Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
Cc: Abhijit Mahajan <abhijit.mahajan@avagotech.com>
Cc: Hariprasad S <hariprasad@chelsio.com>
Cc: Santosh Rastapur <santosh@chelsio.com>
Cc: MPT-FusionLinux.pdl@avagotech.com
Cc: linux-scsi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---

A few implementation notes:

1) Two wrappers are used to simply enable the same prototype
   as expected on modules for module_init()

2) The new helpers are stuffed under kthread.h since including
   kthread.h on init.h caused major issues which are not easy
   to resolve, in fact even including kernel.h in init.h cases
   some issues. We could have keep this under init.h if we ifef'd
   on _LINUX_KTHREAD_H as well but this seems a bit cleaner.

 include/linux/kthread.h | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Comments

Tetsuo Handa Aug. 12, 2014, 10:59 p.m. UTC | #1
Luis R. Rodriguez wrote:
> Tetsuo bisected and found that commit 786235ee \"kthread: make
> kthread_create() killable\" modified kthread_create() to bail as
> soon as SIGKILL is received.

I just wrote commit 786235ee. It is not Tetsuo who bisected it.

> @@ -128,4 +129,38 @@ bool queue_kthread_work(struct kthread_worker *worker,
> void flush_kthread_work(struct kthread_work *work);
> void flush_kthread_worker(struct kthread_worker *worker);
> 
> +#ifndef MODULE
> +
> +#define module_long_probe_init(x)      __initcall(x);
> +#define module_long_probe_exit(x)      __exitcall(x);
> +
> +#else
> +/* To be used by modules which can take over 30 seconds at probe */
> +#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_run(_long_probe_##initfn,\\
> +                             NULL,          \\
> +                             #initfn);          \\
> +          if (IS_ERR(__init_thread))               \\
> +               return PTR_ERR(__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)          \\
> +     {                                   \\
> +          exitfn();                         \\

exitfn() must not be called if initfn() failed or has not
completed yet. You need a bool variable for indicating that
we are ready to call exitfn().

Also, subsequent userspace operations may fail if
we return to userspace before initfn() completes
(e.g. device nodes are not created yet).

> +          if (__init_thread)                    \\
> +               kthread_stop(__init_thread);          \\

We can\'t use kthread_stop() here because we have to
wait for initfn() to succeed before exitfn() is called.

> +     }                                   \\
> +     module_exit(__long_probe_##exitfn);
> +#endif /* MODULE */
> +
> #endif /* _LINUX_KTHREAD_H */
--
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
Greg Kroah-Hartman Aug. 13, 2014, 1:03 a.m. UTC | #2
On Wed, Aug 13, 2014 at 07:59:06AM +0900, Tetsuo Handa wrote:
> Luis R. Rodriguez wrote:
> > Tetsuo bisected and found that commit 786235ee \"kthread: make
> > kthread_create() killable\" modified kthread_create() to bail as
> > soon as SIGKILL is received.
> 
> I just wrote commit 786235ee. It is not Tetsuo who bisected it.
> 
> > @@ -128,4 +129,38 @@ bool queue_kthread_work(struct kthread_worker *worker,
> > void flush_kthread_work(struct kthread_work *work);
> > void flush_kthread_worker(struct kthread_worker *worker);
> > 
> > +#ifndef MODULE
> > +
> > +#define module_long_probe_init(x)      __initcall(x);
> > +#define module_long_probe_exit(x)      __exitcall(x);
> > +
> > +#else
> > +/* To be used by modules which can take over 30 seconds at probe */
> > +#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_run(_long_probe_##initfn,\\
> > +                             NULL,          \\
> > +                             #initfn);          \\
> > +          if (IS_ERR(__init_thread))               \\
> > +               return PTR_ERR(__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)          \\
> > +     {                                   \\
> > +          exitfn();                         \\
> 
> exitfn() must not be called if initfn() failed or has not
> completed yet. You need a bool variable for indicating that
> we are ready to call exitfn().
> 
> Also, subsequent userspace operations may fail if
> we return to userspace before initfn() completes
> (e.g. device nodes are not created yet).

I doubt that this will be a problem, as device nodes are usually created
_after_ module_init() returns.

But the cleanup issues are real on error paths.  Given that these
drivers will need "work" anyway, I don't think it's really a big deal.

thanks,

greg k-h
--
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
Oleg Nesterov Aug. 13, 2014, 5:51 p.m. UTC | #3
On 08/12, Luis R. Rodriguez wrote:
>
> +/* To be used by modules which can take over 30 seconds at probe */

Probably the comment should explain that this hack should only be
used if the driver is buggy and is wating for "real fix".

> +#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_run(_long_probe_##initfn,\
> +					    NULL,		\
> +					    #initfn);		\
> +		if (IS_ERR(__init_thread))			\
> +			return PTR_ERR(__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)		\
> +	{							\
> +		exitfn();					\
> +		if (__init_thread)				\
> +			kthread_stop(__init_thread);		\
> +	}							\

exitfn() should be called after kthread_stop(), and only if initfn()
returns 0. So it should probably do

	int err = kthread_stop(__init_thread);
	if (!err)
		exitfn();

But there is an additional complication, you can't use __init_thread
without get_task_struct(), so  __long_probe_##initfn() can't use
kthread_run(). It needs kthread_create() + get_task_struct() + wakeup.

Oleg.

--
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
Luis R. Rodriguez Aug. 14, 2014, 11:10 p.m. UTC | #4
On Wed, Aug 13, 2014 at 07:51:01PM +0200, Oleg Nesterov wrote:
> On 08/12, Luis R. Rodriguez wrote:
> >
> > +/* To be used by modules which can take over 30 seconds at probe */
> 
> Probably the comment should explain that this hack should only be
> used if the driver is buggy and is wating for "real fix".
> 
> > +#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_run(_long_probe_##initfn,\
> > +					    NULL,		\
> > +					    #initfn);		\
> > +		if (IS_ERR(__init_thread))			\
> > +			return PTR_ERR(__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)		\
> > +	{							\
> > +		exitfn();					\
> > +		if (__init_thread)				\
> > +			kthread_stop(__init_thread);		\
> > +	}							\
> 
> exitfn() should be called after kthread_stop(), and only if initfn()
> returns 0. So it should probably do
> 
> 	int err = kthread_stop(__init_thread);
> 	if (!err)
> 		exitfn();

Thanks! With the check for __init_thread as well as it can be
ERR_PTR(-ENOMEM), ERR_PTR(-EINTR), or NULL (for whatever other
reason).

> But there is an additional complication, you can't use __init_thread
> without get_task_struct(),

Can you elaborate why ? kthread_stop() uses get_task_struct(), 
wake_up_process() and finally put_task_struct(), and we're the
only user of this thread. Also kthread_run() ensures wake_up_process()
gets called on startup, so not sure where the race would be provided
all users here and with the respective helpers on buggy drivers.

> so  __long_probe_##initfn() can't use
> kthread_run(). It needs kthread_create() + get_task_struct() + wakeup.

I fail to see why we'd need to add get_task_struct() on
module_long_probe_init(), can you clarify?

  Luis
--
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
Oleg Nesterov Aug. 15, 2014, 2:39 p.m. UTC | #5
On 08/15, Luis R. Rodriguez wrote:
>
> On Wed, Aug 13, 2014 at 07:51:01PM +0200, Oleg Nesterov wrote:
> > On 08/12, Luis R. Rodriguez wrote:
> > >
> > > +/* To be used by modules which can take over 30 seconds at probe */
> >
> > Probably the comment should explain that this hack should only be
> > used if the driver is buggy and is wating for "real fix".
> >
> > > +#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_run(_long_probe_##initfn,\
> > > +					    NULL,		\
> > > +					    #initfn);		\
> > > +		if (IS_ERR(__init_thread))			\
> > > +			return PTR_ERR(__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)		\
> > > +	{							\
> > > +		exitfn();					\
> > > +		if (__init_thread)				\
> > > +			kthread_stop(__init_thread);		\
> > > +	}							\
> >
> > exitfn() should be called after kthread_stop(), and only if initfn()
> > returns 0. So it should probably do
> >
> > 	int err = kthread_stop(__init_thread);
> > 	if (!err)
> > 		exitfn();
>
> Thanks! With the check for __init_thread as well as it can be
> ERR_PTR(-ENOMEM), ERR_PTR(-EINTR), or NULL (for whatever other
> reason).

Do you mean __long_probe_##exitfn() should also check ERR_PTR(__init_thread)?
I don't think so. If kthread_run() above fails, module_init() should return
the error (it does), so module_exit() won't be called.

> > But there is an additional complication, you can't use __init_thread
> > without get_task_struct(),
>
> Can you elaborate why ? kthread_stop() uses get_task_struct(),

This is too late. This task_struct can be already freed/reused. See below.

> wake_up_process() and finally put_task_struct(), and we're the
> only user of this thread. Also kthread_run() ensures wake_up_process()
> gets called on startup, so not sure where the race would be provided
> all users here and with the respective helpers on buggy drivers.
>
> > so  __long_probe_##initfn() can't use
> > kthread_run(). It needs kthread_create() + get_task_struct() + wakeup.
>
> I fail to see why we'd need to add get_task_struct() on
> module_long_probe_init(), can you clarify?

kthread_stop(kthread_run(callback)) is only safe if callback() can not exit
on its own, without checking kthread_should_stop(). And btw that is why
kthread_stop() does get_task_struct()).

If callback() can exit (if it calls do_exit() or simply returns), then nothing
protects this task_struct, it will be freed.

Oleg.

--
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
Luis R. Rodriguez Aug. 16, 2014, 2:50 a.m. UTC | #6
On Fri, Aug 15, 2014 at 04:39:02PM +0200, Oleg Nesterov wrote:
> On 08/15, Luis R. Rodriguez wrote:
> >
> > On Wed, Aug 13, 2014 at 07:51:01PM +0200, Oleg Nesterov wrote:
> > > On 08/12, Luis R. Rodriguez wrote:
> > > >
> > > > +/* To be used by modules which can take over 30 seconds at probe */
> > >
> > > Probably the comment should explain that this hack should only be
> > > used if the driver is buggy and is wating for "real fix".
> > >
> > > > +#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_run(_long_probe_##initfn,\
> > > > +					    NULL,		\
> > > > +					    #initfn);		\
> > > > +		if (IS_ERR(__init_thread))			\
> > > > +			return PTR_ERR(__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)		\
> > > > +	{							\
> > > > +		exitfn();					\
> > > > +		if (__init_thread)				\
> > > > +			kthread_stop(__init_thread);		\
> > > > +	}							\
> > >
> > > exitfn() should be called after kthread_stop(), and only if initfn()
> > > returns 0. So it should probably do
> > >
> > > 	int err = kthread_stop(__init_thread);
> > > 	if (!err)
> > > 		exitfn();
> >
> > Thanks! With the check for __init_thread as well as it can be
> > ERR_PTR(-ENOMEM), ERR_PTR(-EINTR), or NULL (for whatever other
> > reason).
> 
> Do you mean __long_probe_##exitfn() should also check ERR_PTR(__init_thread)?
> I don't think so. If kthread_run() above fails, module_init() should return
> the error (it does), so module_exit() won't be called.

Good point.

> > > But there is an additional complication, you can't use __init_thread
> > > without get_task_struct(),
> >
> > Can you elaborate why ? kthread_stop() uses get_task_struct(),
> 
> This is too late. This task_struct can be already freed/reused. See below.
> 
> > wake_up_process() and finally put_task_struct(), and we're the
> > only user of this thread. Also kthread_run() ensures wake_up_process()
> > gets called on startup, so not sure where the race would be provided
> > all users here and with the respective helpers on buggy drivers.
> >
> > > so  __long_probe_##initfn() can't use
> > > kthread_run(). It needs kthread_create() + get_task_struct() + wakeup.
> >
> > I fail to see why we'd need to add get_task_struct() on
> > module_long_probe_init(), can you clarify?
> 
> kthread_stop(kthread_run(callback)) is only safe if callback() can not exit
> on its own, without checking kthread_should_stop(). And btw that is why
> kthread_stop() does get_task_struct()).
> 
> If callback() can exit (if it calls do_exit() or simply returns), then nothing
> protects this task_struct, it will be freed.

OK thanks, yeah I see the issue now, and I was able to create a null
pointer dereference by simply calling schedule() quite a bit, will
roll in the required fixes, but come to think of it if there are
other uses (I haven't SmPLd grep'd for grammar uses yet) perhaps
generic helpers would be good? kthread_run_alloc() kthread_run_free().

  Luis
--
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
Takashi Iwai Aug. 17, 2014, 6:59 a.m. UTC | #7
At Sat, 16 Aug 2014 04:50:07 +0200,
Luis R. Rodriguez wrote:
> 
> On Fri, Aug 15, 2014 at 04:39:02PM +0200, Oleg Nesterov wrote:
> > On 08/15, Luis R. Rodriguez wrote:
> > >
> > > On Wed, Aug 13, 2014 at 07:51:01PM +0200, Oleg Nesterov wrote:
> > > > On 08/12, Luis R. Rodriguez wrote:
> > > > >
> > > > > +/* To be used by modules which can take over 30 seconds at probe */
> > > >
> > > > Probably the comment should explain that this hack should only be
> > > > used if the driver is buggy and is wating for "real fix".
> > > >
> > > > > +#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_run(_long_probe_##initfn,\
> > > > > +					    NULL,		\
> > > > > +					    #initfn);		\
> > > > > +		if (IS_ERR(__init_thread))			\
> > > > > +			return PTR_ERR(__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)		\
> > > > > +	{							\
> > > > > +		exitfn();					\
> > > > > +		if (__init_thread)				\
> > > > > +			kthread_stop(__init_thread);		\
> > > > > +	}							\
> > > >
> > > > exitfn() should be called after kthread_stop(), and only if initfn()
> > > > returns 0. So it should probably do
> > > >
> > > > 	int err = kthread_stop(__init_thread);
> > > > 	if (!err)
> > > > 		exitfn();
> > >
> > > Thanks! With the check for __init_thread as well as it can be
> > > ERR_PTR(-ENOMEM), ERR_PTR(-EINTR), or NULL (for whatever other
> > > reason).
> > 
> > Do you mean __long_probe_##exitfn() should also check ERR_PTR(__init_thread)?
> > I don't think so. If kthread_run() above fails, module_init() should return
> > the error (it does), so module_exit() won't be called.
> 
> Good point.
> 
> > > > But there is an additional complication, you can't use __init_thread
> > > > without get_task_struct(),
> > >
> > > Can you elaborate why ? kthread_stop() uses get_task_struct(),
> > 
> > This is too late. This task_struct can be already freed/reused. See below.
> > 
> > > wake_up_process() and finally put_task_struct(), and we're the
> > > only user of this thread. Also kthread_run() ensures wake_up_process()
> > > gets called on startup, so not sure where the race would be provided
> > > all users here and with the respective helpers on buggy drivers.
> > >
> > > > so  __long_probe_##initfn() can't use
> > > > kthread_run(). It needs kthread_create() + get_task_struct() + wakeup.
> > >
> > > I fail to see why we'd need to add get_task_struct() on
> > > module_long_probe_init(), can you clarify?
> > 
> > kthread_stop(kthread_run(callback)) is only safe if callback() can not exit
> > on its own, without checking kthread_should_stop(). And btw that is why
> > kthread_stop() does get_task_struct()).
> > 
> > If callback() can exit (if it calls do_exit() or simply returns), then nothing
> > protects this task_struct, it will be freed.
> 
> OK thanks, yeah I see the issue now, and I was able to create a null
> pointer dereference by simply calling schedule() quite a bit, will
> roll in the required fixes, but come to think of it if there are
> other uses (I haven't SmPLd grep'd for grammar uses yet) perhaps
> generic helpers would be good? kthread_run_alloc() kthread_run_free().

How about just increasing/decreasing the module count for blocking the
exit call?  For example:

#define module_long_probe_init(initfn)				\
	static int _long_probe_##initfn(void *arg)		\
	{							\
		int ret = initfn();				\
		module_put(THIS_MODULE);			\
		return ret;					\
	}							\
	static inline __init int __long_probe_##initfn(void)	\
	{							\
		struct task_struct *__init_thread;		\
		__module_get(THIS_MODULE);			\
		__init_thread = kthread_run(_long_probe_##initfn,\
					    NULL,		\
					    #initfn);		\
		if (IS_ERR(__init_thread)) {			\
			module_put(THIS_MODULE);		\
			return PTR_ERR(__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)				\
	module_exit(exitfn);



Takashi
--
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
Oleg Nesterov Aug. 17, 2014, 12:25 p.m. UTC | #8
On 08/17, Takashi Iwai wrote:
>
> How about just increasing/decreasing the module count for blocking the
> exit call?  For example:
>
> #define module_long_probe_init(initfn)				\
> 	static int _long_probe_##initfn(void *arg)		\
> 	{							\
> 		int ret = initfn();				\
> 		module_put(THIS_MODULE);			\

WINDOW, please see below.

> 		return ret;					\
> 	}							\
> 	static inline __init int __long_probe_##initfn(void)	\
> 	{							\
> 		struct task_struct *__init_thread;		\
> 		__module_get(THIS_MODULE);			\
> 		__init_thread = kthread_run(_long_probe_##initfn,\
> 					    NULL,		\
> 					    #initfn);		\
> 		if (IS_ERR(__init_thread)) {			\
> 			module_put(THIS_MODULE);		\
> 			return PTR_ERR(__init_thread);		\
> 		}						\
> 		return 0;					\
> 	}							\

I leave this to you and Luis, but personally I think this is very
nice idea, I like it. Because sys_delete_module() won't hang in D
state waiting for initfn().

There is a small problem. This module can be unloaded right after
module_put() above. In this case its memory can be unmapped and
the exiting thread can crash.

This is very unlikely, this thread needs to execute just a few insn
and escape from this module's memory. Given that only the buggy
modules should use this hack, perhaps we can even ignore this race.

But perhaps it makes sense to close this race anyway, and we already
have complete_and_exit() which can be used instead of "return ret"
above. Just we need the additional "static struct completion" and
module_exit() should call wait_for_completion.

Oleg.

--
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
Oleg Nesterov Aug. 17, 2014, 12:48 p.m. UTC | #9
On 08/17, Oleg Nesterov wrote:
>
> On 08/17, Takashi Iwai wrote:
> >
> > How about just increasing/decreasing the module count for blocking the
> > exit call?  For example:
> >
> > #define module_long_probe_init(initfn)				\
> > 	static int _long_probe_##initfn(void *arg)		\
> > 	{							\
> > 		int ret = initfn();				\
> > 		module_put(THIS_MODULE);			\
>
> WINDOW, please see below.
>
> > 		return ret;					\
> > 	}							\
> > 	static inline __init int __long_probe_##initfn(void)	\
> > 	{							\
> > 		struct task_struct *__init_thread;		\
> > 		__module_get(THIS_MODULE);			\
> > 		__init_thread = kthread_run(_long_probe_##initfn,\
> > 					    NULL,		\
> > 					    #initfn);		\
> > 		if (IS_ERR(__init_thread)) {			\
> > 			module_put(THIS_MODULE);		\
> > 			return PTR_ERR(__init_thread);		\
> > 		}						\
> > 		return 0;					\
> > 	}							\
>
> I leave this to you and Luis, but personally I think this is very
> nice idea, I like it. Because sys_delete_module() won't hang in D
> state waiting for initfn().
>
> There is a small problem. This module can be unloaded right after
> module_put() above. In this case its memory can be unmapped and
> the exiting thread can crash.
>
> This is very unlikely, this thread needs to execute just a few insn
> and escape from this module's memory. Given that only the buggy
> modules should use this hack, perhaps we can even ignore this race.
>
> But perhaps it makes sense to close this race anyway, and we already
> have complete_and_exit() which can be used instead of "return ret"
> above. Just we need the additional "static struct completion" and
> module_exit() should call wait_for_completion.

Forgot to mention... and __long_probe_##initfn() could be simpler
without kthread_run,

	__init_thread = kthread_create(...);
	if (IS_ERR(__init_thread))
		return PTR_ERR();

	module_get(THIS_MODULE);
	wake_up_process(__init_thread);
	return 0;

but this is subjective, up to you.

Oleg.

--
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
Oleg Nesterov Aug. 17, 2014, 12:55 p.m. UTC | #10
Damn, sorry for noise ;)

I was going to suggest to introduce module_put_and_exit() to simplify
this and potentially other users, but it already exists. So this code
can use it too without additional complications.

On 08/17, Oleg Nesterov wrote:
> On 08/17, Oleg Nesterov wrote:
> >
> > On 08/17, Takashi Iwai wrote:
> > >
> > > How about just increasing/decreasing the module count for blocking the
> > > exit call?  For example:
> > >
> > > #define module_long_probe_init(initfn)				\
> > > 	static int _long_probe_##initfn(void *arg)		\
> > > 	{							\
> > > 		int ret = initfn();				\
> > > 		module_put(THIS_MODULE);			\
> >
> > WINDOW, please see below.
> >
> > > 		return ret;					\
> > > 	}							\
> > > 	static inline __init int __long_probe_##initfn(void)	\
> > > 	{							\
> > > 		struct task_struct *__init_thread;		\
> > > 		__module_get(THIS_MODULE);			\
> > > 		__init_thread = kthread_run(_long_probe_##initfn,\
> > > 					    NULL,		\
> > > 					    #initfn);		\
> > > 		if (IS_ERR(__init_thread)) {			\
> > > 			module_put(THIS_MODULE);		\
> > > 			return PTR_ERR(__init_thread);		\
> > > 		}						\
> > > 		return 0;					\
> > > 	}							\
> >
> > I leave this to you and Luis, but personally I think this is very
> > nice idea, I like it. Because sys_delete_module() won't hang in D
> > state waiting for initfn().
> >
> > There is a small problem. This module can be unloaded right after
> > module_put() above. In this case its memory can be unmapped and
> > the exiting thread can crash.
> >
> > This is very unlikely, this thread needs to execute just a few insn
> > and escape from this module's memory. Given that only the buggy
> > modules should use this hack, perhaps we can even ignore this race.
> >
> > But perhaps it makes sense to close this race anyway, and we already
> > have complete_and_exit() which can be used instead of "return ret"
> > above. Just we need the additional "static struct completion" and
> > module_exit() should call wait_for_completion.
> 
> Forgot to mention... and __long_probe_##initfn() could be simpler
> without kthread_run,
> 
> 	__init_thread = kthread_create(...);
> 	if (IS_ERR(__init_thread))
> 		return PTR_ERR();
> 
> 	module_get(THIS_MODULE);
> 	wake_up_process(__init_thread);
> 	return 0;
> 
> but this is subjective, up to you.
> 
> Oleg.

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

Patch

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 13d5520..2b5555a 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -1,6 +1,7 @@ 
 #ifndef _LINUX_KTHREAD_H
 #define _LINUX_KTHREAD_H
 /* Simple interface for creating and stopping kernel threads without mess. */
+#include <linux/init.h>
 #include <linux/err.h>
 #include <linux/sched.h>
 
@@ -128,4 +129,38 @@  bool queue_kthread_work(struct kthread_worker *worker,
 void flush_kthread_work(struct kthread_work *work);
 void flush_kthread_worker(struct kthread_worker *worker);
 
+#ifndef MODULE
+
+#define module_long_probe_init(x)      __initcall(x);
+#define module_long_probe_exit(x)      __exitcall(x);
+
+#else
+/* To be used by modules which can take over 30 seconds at probe */
+#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_run(_long_probe_##initfn,\
+					    NULL,		\
+					    #initfn);		\
+		if (IS_ERR(__init_thread))			\
+			return PTR_ERR(__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)		\
+	{							\
+		exitfn();					\
+		if (__init_thread)				\
+			kthread_stop(__init_thread);		\
+	}							\
+	module_exit(__long_probe_##exitfn);
+#endif /* MODULE */
+
 #endif /* _LINUX_KTHREAD_H */