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.
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
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
> 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 <[email protected]>
---
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)
{
On Tue, 27 Jan 2009 14:09:05 +0900 (JST) KOSAKI Motohiro <[email protected]> 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 <[email protected]>
> ---
> 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.
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... ;-)
On Tue, 27 Jan 2009 00:19:19 -0500 Kyle McMartin <[email protected]> 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
:(
On Mon, 26 Jan 2009 21:26:15 -0800 Andrew Morton <[email protected]> wrote:
> On Tue, 27 Jan 2009 00:19:19 -0500 Kyle McMartin <[email protected]> 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()".
From: Kyle McMartin <[email protected]>
Suggest using the get_task_comm accessor versus direct access to
current->comm.
Signed-off-by: Kyle McMartin <[email protected]>
---
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/ &&
> +# 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.
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
> 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.
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
On Mon, Jan 26, 2009 at 09:29:18PM -0800, Andrew Morton wrote:
> On Mon, 26 Jan 2009 21:26:15 -0800 Andrew Morton <[email protected]> wrote:
>
> > On Tue, 27 Jan 2009 00:19:19 -0500 Kyle McMartin <[email protected]> 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.
On Tue, 27 Jan 2009 11:49:56 +0300 Alexey Dobriyan <[email protected]> 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 <[email protected]> wrote:
> >
> > > On Tue, 27 Jan 2009 00:19:19 -0500 Kyle McMartin <[email protected]> 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.
> > > 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 ;)
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
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
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
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