get_task_comm() not exported?

get_task_comm() not exported?

[Christoph Bartelmus]

Hi,

is there any reason why get_task_comm() is not exported in fs/exec.c?

I'd like to use get_task_comm() from a kernel module and get unresolved  
symbol errors.

Adding EXPORT_SYMBOL(get_task_comm); to fs/exec.c fixes the problem.

Cheers,
Christoph

Please Cc.

Re: get_task_comm() not exported?

[KOSAKI Motohiro]

Hi

> Hi,
> 
> is there any reason why get_task_comm() is not exported in fs/exec.c?

In general, the only function of anybody necessarity explained is exported.
if you want to export get_task_comm(), you need to explain reasonable reason.

> 
> I'd like to use get_task_comm() from a kernel module and get unresolved  
> symbol errors.
> 
> Adding EXPORT_SYMBOL(get_task_comm); to fs/exec.c fixes the problem.
> 
> Cheers,
> Christoph
> 
> Please Cc.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: get_task_comm() not exported?

[Christoph Bartelmus]

Hi,

on 19 Jan 09 at 07:36, KOSAKI Motohiro wrote:
>> is there any reason why get_task_comm() is not exported in fs/exec.c?

> In general, the only function of anybody necessarity explained is exported.
> if you want to export get_task_comm(), you need to explain reasonable
> reason.

It's nothing that important: just want to print the executable name to the  
logs during error handling. get_task_comm() is the required accessor  
function.

http://lirc.cvs.sourceforge.net/viewvc/lirc/lirc/drivers/lirc_dev/lirc_dev.c?revision=1.70&view=markup

Cheers,
Christoph

[PATCH] export get_task_comm()

[KOSAKI Motohiro]

> Hi,
> 
> on 19 Jan 09 at 07:36, KOSAKI Motohiro wrote:
> >> is there any reason why get_task_comm() is not exported in fs/exec.c?
> 
> > In general, the only function of anybody necessarity explained is exported.
> > if you want to export get_task_comm(), you need to explain reasonable
> > reason.
> 
> It's nothing that important: just want to print the executable name to the  
> logs during error handling. get_task_comm() is the required accessor  
> function.
> 
> http://lirc.cvs.sourceforge.net/viewvc/lirc/lirc/drivers/lirc_dev/lirc_dev.c?revision=1.70&view=markup

To be honest, I don't use get_task_comm(). but I don't oppose your request.

==
Subject: [PATCH] export get_task_comm()

task::comm is good debugging information and driver developer want to
use this information easily.

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 fs/exec.c |    1 +
 1 file changed, 1 insertion(+)

Index: b/fs/exec.c
===================================================================
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -936,6 +936,7 @@ char *get_task_comm(char *buf, struct ta
 	task_unlock(tsk);
 	return buf;
 }
+EXPORT_SYMBOL_GPL(get_task_comm);
 
 void set_task_comm(struct task_struct *tsk, char *buf)
 {

Re: [PATCH] export get_task_comm()

[Andrew Morton]

On Tue, 27 Jan 2009 14:09:05 +0900 (JST) KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> > Hi,
> > 
> > on 19 Jan 09 at 07:36, KOSAKI Motohiro wrote:
> > >> is there any reason why get_task_comm() is not exported in fs/exec.c?
> > 
> > > In general, the only function of anybody necessarity explained is exported.
> > > if you want to export get_task_comm(), you need to explain reasonable
> > > reason.
> > 
> > It's nothing that important: just want to print the executable name to the  
> > logs during error handling. get_task_comm() is the required accessor  
> > function.
> > 
> > http://lirc.cvs.sourceforge.net/viewvc/lirc/lirc/drivers/lirc_dev/lirc_dev.c?revision=1.70&view=markup
> 
> To be honest, I don't use get_task_comm(). but I don't oppose your request.
> 
> ==
> Subject: [PATCH] export get_task_comm()
> 
> task::comm is good debugging information and driver developer want to
> use this information easily.
> 
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> ---
>  fs/exec.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: b/fs/exec.c
> ===================================================================
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -936,6 +936,7 @@ char *get_task_comm(char *buf, struct ta
>  	task_unlock(tsk);
>  	return buf;
>  }
> +EXPORT_SYMBOL_GPL(get_task_comm);
>  
>  void set_task_comm(struct task_struct *tsk, char *buf)
>  {

Ho hum, I suppose so.  I redid the changelog a bit:

   task_struct.comm[] is useful for debugging and driver developers
   want to use this information easily.  Direct access to
   task_struct.comm[] is a bit racy, so export the official accessor.

Re: [PATCH] export get_task_comm()

[Kyle McMartin]

On Mon, Jan 26, 2009 at 09:16:55PM -0800, Andrew Morton wrote:
> Ho hum, I suppose so.  I redid the changelog a bit:
> 
>    task_struct.comm[] is useful for debugging and driver developers
>    want to use this information easily.  Direct access to
>    task_struct.comm[] is a bit racy, so export the official accessor.
> 

Maybe lirc should be submitted to staging/ before we go exporting
symbols for out of tree things... ;-)

Re: [PATCH] export get_task_comm()

[Andrew Morton]

On Tue, 27 Jan 2009 00:19:19 -0500 Kyle McMartin <kyle@infradead.org> wrote:

> On Mon, Jan 26, 2009 at 09:16:55PM -0800, Andrew Morton wrote:
> > Ho hum, I suppose so.  I redid the changelog a bit:
> > 
> >    task_struct.comm[] is useful for debugging and driver developers
> >    want to use this information easily.  Direct access to
> >    task_struct.comm[] is a bit racy, so export the official accessor.
> > 
> 
> Maybe lirc should be submitted to staging/ before we go exporting
> symbols for out of tree things... ;-)

y:/usr/src/linux-2.6.29-rc2> grep -r 'current->comm' drivers | wc -l
77

:(

Re: [PATCH] export get_task_comm()

[Andrew Morton]

On Mon, 26 Jan 2009 21:26:15 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 27 Jan 2009 00:19:19 -0500 Kyle McMartin <kyle@infradead.org> wrote:
> 
> > On Mon, Jan 26, 2009 at 09:16:55PM -0800, Andrew Morton wrote:
> > > Ho hum, I suppose so.  I redid the changelog a bit:
> > > 
> > >    task_struct.comm[] is useful for debugging and driver developers
> > >    want to use this information easily.  Direct access to
> > >    task_struct.comm[] is a bit racy, so export the official accessor.
> > > 
> > 
> > Maybe lirc should be submitted to staging/ before we go exporting
> > symbols for out of tree things... ;-)
> 
> y:/usr/src/linux-2.6.29-rc2> grep -r 'current->comm' drivers | wc -l
> 77
> 
> :(

It's worth a checkpatch rule, I guess: "direct access to
task_struct.comm is racy - use get_task_comm()".

[PATCH] make checkpatch warn about access to current->comm

[Kyle McMartin]

From: Kyle McMartin <kyle@redhat.com>

Suggest using the get_task_comm accessor versus direct access to
current->comm.

Signed-off-by: Kyle McMartin <kyle@redhat.com>

---

I just mashed it in the middle of existing checks to minimize risk of
collisions.

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 45eb0ae..4f2da5d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2533,6 +2533,11 @@ sub process {
 				$herecurr);
 		}
 
+# direct access to current task name is racy, suggest accessor instead.
+		if ($line =~ /current\-\>comm/) {
+			WARN("direct access to current->comm is racy, use get_task_comm() instead.\n" . $herecurr);
+		}
+
 # use of NR_CPUS is usually wrong
 # ignore definitions of NR_CPUS and usage to define arrays as likely right
 		if ($line =~ /\bNR_CPUS\b/ &&

Re: [PATCH] make checkpatch warn about access to current->comm

[KOSAKI Motohiro]

> +# direct access to current task name is racy, suggest accessor instead.
> +		if ($line =~ /current\-\>comm/) {
> +			WARN("direct access to current->comm is racy, use get_task_comm() instead.\n" . $herecurr);
> +		}
> +
>  # use of NR_CPUS is usually wrong
>  # ignore definitions of NR_CPUS and usage to define arrays as likely right
>  		if ($line =~ /\bNR_CPUS\b/ &&


./scripts/checkpatch --file fs/exec.c
------------------------------------------------------
WARNING: direct access to current->comm is racy, use get_task_comm() instead.
#952: FILE: exec.c:952:
+       char tcomm[sizeof(current->comm)];

(snip)

WARNING: direct access to current->comm is racy, use get_task_comm() instead.
#1459: FILE: exec.c:1459:
+                                             "%s", current->comm);

(snip)

WARNING: direct access to current->comm is racy, use get_task_comm() instead.
#1788: FILE: exec.c:1788:
+               if (!strcmp(delimit, current->comm)) {

---------------------------

I think "char tcomm[sizeof(current->comm)];" is valid code.
if checkpatch.pl don't warn "sizeof(current->comm)", I'm glad.

Re: [PATCH] make checkpatch warn about access to current->comm

[Kyle McMartin]

On Tue, Jan 27, 2009 at 02:50:18PM +0900, KOSAKI Motohiro wrote:
> I think "char tcomm[sizeof(current->comm)];" is valid code.
> if checkpatch.pl don't warn "sizeof(current->comm)", I'm glad.
> 

Awfully interesting way of writing TASK_COMM_LEN :)

cheers, Kyle

Re: [PATCH] make checkpatch warn about access to current->comm

[KOSAKI Motohiro]

> On Tue, Jan 27, 2009 at 02:50:18PM +0900, KOSAKI Motohiro wrote:
> > I think "char tcomm[sizeof(current->comm)];" is valid code.
> > if checkpatch.pl don't warn "sizeof(current->comm)", I'm glad.
> 
> Awfully interesting way of writing TASK_COMM_LEN :)

I don't think so awfully.
I think "sizeof(array_val)" is typical code in kernel.

I agree that we can rewrite s/tcomm[sizeof(current->comm)]/char tcomm[TASK_COMM_LEN]/.
but it's annoyed.

Re: [PATCH] make checkpatch warn about access to current->comm

[Kyle McMartin]

On Tue, Jan 27, 2009 at 02:58:51PM +0900, KOSAKI Motohiro wrote:
> > On Tue, Jan 27, 2009 at 02:50:18PM +0900, KOSAKI Motohiro wrote:
> > > I think "char tcomm[sizeof(current->comm)];" is valid code.
> > > if checkpatch.pl don't warn "sizeof(current->comm)", I'm glad.
> > 
> > Awfully interesting way of writing TASK_COMM_LEN :)
> 
> I don't think so awfully.
> I think "sizeof(array_val)" is typical code in kernel.
> 
> I agree that we can rewrite s/tcomm[sizeof(current->comm)]/char tcomm[TASK_COMM_LEN]/.
> but it's annoyed.
> 

If current->comm was changed to be kmalloc'd this would need to be
changed anyway, so why not just do it now?

regards, Kyle

Re: [PATCH] make checkpatch warn about access to current->comm

[Linus Torvalds]

On Tue, 27 Jan 2009, Kyle McMartin wrote:
> 
> Suggest using the get_task_comm accessor versus direct access to
> current->comm.

I think "current->comm" is fine, and not racy.

It only gets racy when you ask for the name of _another_ task.

And quite frankly, I don't think anybody but /proc does that anyway. I 
think this whole "get_task_comm()" thing is overrated. Most people are 
better off doing just "current->comm".

		Linus

Re: [PATCH] make checkpatch warn about access to current->comm

[Kyle McMartin]

On Tue, Jan 27, 2009 at 07:45:41AM -0800, Linus Torvalds wrote:
> 
> 
> On Tue, 27 Jan 2009, Kyle McMartin wrote:
> > 
> > Suggest using the get_task_comm accessor versus direct access to
> > current->comm.
> 
> I think "current->comm" is fine, and not racy.
> 
> It only gets racy when you ask for the name of _another_ task.
> 
> And quite frankly, I don't think anybody but /proc does that anyway. I 
> think this whole "get_task_comm()" thing is overrated. Most people are 
> better off doing just "current->comm".
> 

Sure, fine by me. I'd forgotten that prctl doesn't have a `pid' argument
to change another tasks comm.

regards, Kyle

Re: [PATCH] make checkpatch warn about access to current->comm

[Christoph Bartelmus]

On 27 Jan 09 at 07:45, Linus Torvalds wrote:
> On Tue, 27 Jan 2009, Kyle McMartin wrote:
>>
>> Suggest using the get_task_comm accessor versus direct access to
>> current->comm.

> I think "current->comm" is fine, and not racy.
>
> It only gets racy when you ask for the name of _another_ task.
>
> And quite frankly, I don't think anybody but /proc does that anyway. I
> think this whole "get_task_comm()" thing is overrated. Most people are
> better off doing just "current->comm".

This issue only came up because for someone like me it's not obvious at  
all that using "current->comm" is safe and the comment in sched.h  
explicitly points out that task_struct.comm should be accessed with  
[gs]et_task_comm.

Christoph

Re: [PATCH] export get_task_comm()

[Alexey Dobriyan]

On Mon, Jan 26, 2009 at 09:29:18PM -0800, Andrew Morton wrote:
> On Mon, 26 Jan 2009 21:26:15 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Tue, 27 Jan 2009 00:19:19 -0500 Kyle McMartin <kyle@infradead.org> wrote:
> > 
> > > On Mon, Jan 26, 2009 at 09:16:55PM -0800, Andrew Morton wrote:
> > > > Ho hum, I suppose so.  I redid the changelog a bit:
> > > > 
> > > >    task_struct.comm[] is useful for debugging and driver developers
> > > >    want to use this information easily.  Direct access to
> > > >    task_struct.comm[] is a bit racy, so export the official accessor.
> > > > 
> > > 
> > > Maybe lirc should be submitted to staging/ before we go exporting
> > > symbols for out of tree things... ;-)
> > 
> > y:/usr/src/linux-2.6.29-rc2> grep -r 'current->comm' drivers | wc -l
> > 77
> > 
> > :(
> 
> It's worth a checkpatch rule, I guess: "direct access to
> task_struct.comm is racy - use get_task_comm()".

And majority of usages is some debugging printk where nobody cares if ->comm
corrupted.

Changelog says is useful for debugging. That's right, tsk->comm is useful
for debugging, not allocating temporary buffer + get_task_comm().

Some ->comm usages are for kernel threads which never change it, for starters.

current->comm is always safe, because, current is not executing prctl(2)! 

I'd say nothing should be done and, heavens forbid, adding this to checkpatch.pl.

Re: [PATCH] export get_task_comm()

[Andrew Morton]

On Tue, 27 Jan 2009 11:49:56 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Mon, Jan 26, 2009 at 09:29:18PM -0800, Andrew Morton wrote:
> > On Mon, 26 Jan 2009 21:26:15 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > On Tue, 27 Jan 2009 00:19:19 -0500 Kyle McMartin <kyle@infradead.org> wrote:
> > > 
> > > > On Mon, Jan 26, 2009 at 09:16:55PM -0800, Andrew Morton wrote:
> > > > > Ho hum, I suppose so.  I redid the changelog a bit:
> > > > > 
> > > > >    task_struct.comm[] is useful for debugging and driver developers
> > > > >    want to use this information easily.  Direct access to
> > > > >    task_struct.comm[] is a bit racy, so export the official accessor.
> > > > > 
> > > > 
> > > > Maybe lirc should be submitted to staging/ before we go exporting
> > > > symbols for out of tree things... ;-)
> > > 
> > > y:/usr/src/linux-2.6.29-rc2> grep -r 'current->comm' drivers | wc -l
> > > 77
> > > 
> > > :(
> > 
> > It's worth a checkpatch rule, I guess: "direct access to
> > task_struct.comm is racy - use get_task_comm()".
> 
> And majority of usages is some debugging printk where nobody cares if ->comm
> corrupted.
> 
> Changelog says is useful for debugging. That's right, tsk->comm is useful
> for debugging, not allocating temporary buffer + get_task_comm().
> 
> Some ->comm usages are for kernel threads which never change it, for starters.
> 
> current->comm is always safe, because, current is not executing prctl(2)! 

That's a good point.

> I'd say nothing should be done and, heavens forbid, adding this to checkpatch.pl.

hm, yeah, OK, I'll drop it.

Re: [PATCH] export get_task_comm()

[KOSAKI Motohiro]

> > > It's worth a checkpatch rule, I guess: "direct access to
> > > task_struct.comm is racy - use get_task_comm()".
> > 
> > And majority of usages is some debugging printk where nobody cares if ->comm
> > corrupted.
> > 
> > Changelog says is useful for debugging. That's right, tsk->comm is useful
> > for debugging, not allocating temporary buffer + get_task_comm().
> > 
> > Some ->comm usages are for kernel threads which never change it, for starters.
> > 
> > current->comm is always safe, because, current is not executing prctl(2)! 
> 
> That's a good point.
> 
> > I'd say nothing should be done and, heavens forbid, adding this to checkpatch.pl.
> 
> hm, yeah, OK, I'll drop it.

Oh, I have to agree Alexey because he is always right ;)

Re: [PATCH] export get_task_comm()

[Linus Torvalds]

On Mon, 26 Jan 2009, Andrew Morton wrote:
> > 
> > task::comm is good debugging information and driver developer want to
> > use this information easily.
> 
> Ho hum, I suppose so.  I redid the changelog a bit:
> 
>    task_struct.comm[] is useful for debugging and driver developers
>    want to use this information easily.  Direct access to
>    task_struct.comm[] is a bit racy, so export the official accessor.

The biggest issue I have with this is that the whole "get_task_comm()" 
interface is not very good for random users - it inherently depends on the 
result buffer being at least sizeof(tsk->comm).

If we export it to random routines, I get the feeling that we should pass 
in the size of the result buffer, so that they don't have to know about 
this requirement.

		Linus