Bug 849364 - ptrace.c missing #define of EXPORT_SYMBOL_GPL, moved from regset.h to export.h
ptrace.c missing #define of EXPORT_SYMBOL_GPL, moved from regset.h to export.h
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: kernel (Show other bugs)
17
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Kernel Maintainer List
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-18 16:00 EDT by stan
Modified: 2012-10-10 11:12 EDT (History)
11 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-10-10 11:12:06 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description stan 2012-08-18 16:00:09 EDT
Description of problem:
During compile I see the following messages:
arch/x86/kernel/ptrace.c:1422:1: warning: data definition has no type or storage class [enabled by default]
arch/x86/kernel/ptrace.c:1422:1: warning: type defaults to 'int' in declaration of 'EXPORT_SYMBOL_GPL' [-Wimplicit-int]
arch/x86/kernel/ptrace.c:1422:1: warning: parameter names (without types) in function declaration [enabled by default]
They are happening because of a missing #define, because it has moved to a different file.  See below for more detail.

Version-Release number of selected component (if applicable):
3.5.2-1, but I think it has been there for all the 3.5 series

How reproducible:
every time.

Steps to Reproduce:
1.  Download the src.rpm
2.  Custom configure it.
3.  Compile it.
  
Actual results:
Warning is present

Expected results:
No warning

Additional info:
The code it ptrace.c where this occurs has a comment:
/*
 * This is declared in linux/regset.h and defined in machine-dependent
 * code.  We put the export here to ensure no machine forgets it.
 */
EXPORT_SYMBOL_GPL(task_user_regset_view);

This is no longer defined in regset.h, it is now in export.h.  This file arch/x86/kernel/ptrace.c needs an   #include {linux/export.h}   added to remove this error.

The kernels compiled with this problem seem to boot and run fine, but I have experienced some lockups of X with the 3.5 series kernels, with none in earlier kernels.  I haven't tried the fix yet, next kernel you release I'll patch it in.
Comment 1 Josh Boyer 2012-08-20 09:33:05 EDT
This comes from the uprobes-backport.patch we carry.  Anton, Oleg, is there a reason this symbol is exported?  I don't see it exported in -tip anywhere.
Comment 2 Oleg Nesterov 2012-08-20 13:21:26 EDT
(In reply to comment #1)
>
> This comes from the uprobes-backport.patch we carry.

And obviously due to mistake ;)

Cough, sorry... Could you please remind me where can I find
Anton's backport patches??? He is on vacation till the next
week.
Comment 3 Josh Boyer 2012-08-20 13:34:28 EDT
(In reply to comment #2)
> (In reply to comment #1)
> >
> > This comes from the uprobes-backport.patch we carry.
> 
> And obviously due to mistake ;)
> 
> Cough, sorry... Could you please remind me where can I find
> Anton's backport patches??? He is on vacation till the next
> week.

The main patch is in Fedora 17 git as uprobes-backport.patch.  I believe the split out versions are here:

 git://fedorapeople.org/home/fedora/aarapov/public_git/kernel-uprobes.git tags/17_exported
Comment 4 Oleg Nesterov 2012-08-20 13:41:47 EDT
(In reply to comment #2)
>
> (In reply to comment #1)
> >
> > This comes from the uprobes-backport.patch we carry.
> 
> And obviously due to mistake ;)

Probably I was wrong,

> Cough, sorry... Could you please remind me where can I find
> Anton's backport patches??? He is on vacation till the next
> week.

Found it... can't verify for sure right now, but it seems this
was added by "uprobes: add exports necessary for uprobes use by
modules" from Josh Stone.
Comment 5 Oleg Nesterov 2012-08-20 13:49:23 EDT
(In reply to comment #0)
>
> The code it ptrace.c where this occurs has a comment:
> /*
>  * This is declared in linux/regset.h and defined in machine-dependent
>  * code.  We put the export here to ensure no machine forgets it.
>  */
> EXPORT_SYMBOL_GPL(task_user_regset_view);
> 
> This is no longer defined in regset.h, it is now in export.h.

I was confused by this part. The comment means task_user_regset_view,
not EXPORT_SYMBOL_GPL, it is still in regset.h.

> This file
> arch/x86/kernel/ptrace.c needs an   #include {linux/export.h}   added to
> remove this error.

Or we should move this EXPORT_ to kernel/ptrace.c, this will even
match the "ensure no machine forgets it" comment above ;)
Comment 6 Josh Boyer 2012-08-20 13:55:24 EDT
(In reply to comment #4)
> (In reply to comment #2)
> >
> > (In reply to comment #1)
> > >
> > > This comes from the uprobes-backport.patch we carry.
> > 
> > And obviously due to mistake ;)
> 
> Probably I was wrong,
> 
> > Cough, sorry... Could you please remind me where can I find
> > Anton's backport patches??? He is on vacation till the next
> > week.
> 
> Found it... can't verify for sure right now, but it seems this
> was added by "uprobes: add exports necessary for uprobes use by
> modules" from Josh Stone.

Is that going upstream soon?
Comment 7 Oleg Nesterov 2012-08-20 13:57:47 EDT
(In reply to comment #6)
> 
> Is that going upstream soon?

I guess no.
Comment 8 Josh Boyer 2012-08-20 14:00:00 EDT
(In reply to comment #7)
> (In reply to comment #6)
> > 
> > Is that going upstream soon?
> 
> I guess no.

:\

Is there other work on-going to make it unnecessary?  The rest of this patch is backports from upstream, and once we rebase most of them should go away.  Having to carry around a patch to export symbols for systemtap seems sub-optimal.
Comment 9 Josh Stone 2012-08-20 14:06:09 EDT
(In reply to comment #4)
> Found it... can't verify for sure right now, but it seems this
> was added by "uprobes: add exports necessary for uprobes use by
> modules" from Josh Stone.

The export "necessity" is a somewhat soft requirement.  We do need these functions from modules for systemtap, and exporting is the *right* way to get to them.  We do have a fallback to use kallsyms_lookup_name when it's not exported though.  And since those warnings make the export look like a function decl, then we've apparently been using only the fallback for a while. :/

Including export.h as comment #0 suggests should work.  I see in the build log that arch/x86/kernel/uprobes.c also has this warning and needs this fix.

Actually, I think my exports patch originally put these in kernel/ptrace.c and kernel/uprobes.c, and I seem to recall Anton fixed this export.h once too.  Somewhere along the carried patches I guess this got shuffled and lost.
Comment 10 Oleg Nesterov 2012-08-21 08:52:03 EDT
(In reply to comment #9)
>
> Actually, I think my exports patch originally put these in kernel/ptrace.c

Yes, this looks more natural and correct

> and I seem to recall Anton fixed this export.h once

and in this case no more changes are needed.
Comment 11 stan 2012-08-25 21:33:57 EDT
I can confirm that adding the include of export.h to ptrace.c took care of the warning during the compile of 3.5.2-3, and the resulting kernel is running just fine.
Comment 12 Anton Arapov 2012-08-30 10:55:00 EDT
(In reply to comment #9)

  Josh, I'm a bit lost(didn't read carefully :) ) in all this discussion... I will pay attention to this issue before I will roll out next uprobes patch update (plan to do it next week). 
  Is there anything you want to do me immediately about this?

Anton.
Comment 13 Josh Stone 2012-09-07 14:00:24 EDT
(In reply to comment #12)
>   Josh, I'm a bit lost(didn't read carefully :) ) in all this discussion...
> I will pay attention to this issue before I will roll out next uprobes patch
> update (plan to do it next week). 
>   Is there anything you want to do me immediately about this?

Either make sure that the places we add EXPORT_SYMBOL_GPL all have export.h, or just drop the exports altogether.

I prefer keeping the exports, because that's the main form of kernel agreement that these functions are ok for us to use.  But we can get to them anyway through kallsyms if necessary.
Comment 14 Josh Boyer 2012-09-07 15:57:09 EDT
(In reply to comment #13)
> (In reply to comment #12)
> >   Josh, I'm a bit lost(didn't read carefully :) ) in all this discussion...
> > I will pay attention to this issue before I will roll out next uprobes patch
> > update (plan to do it next week). 
> >   Is there anything you want to do me immediately about this?
> 
> Either make sure that the places we add EXPORT_SYMBOL_GPL all have export.h,
> or just drop the exports altogether.
> 
> I prefer keeping the exports, because that's the main form of kernel
> agreement that these functions are ok for us to use.  But we can get to them
> anyway through kallsyms if necessary.

Not to be all grumpy, but "agreement" is kind of an overloaded word there.  If you're adding an EXPORT_SYMBOL (gpl or not) to the Fedora kernel but that isn't going to be sent or accepted upstream (per comment #7), that's kind of not agreement.

If you really need the exports then by all means keep them.  However, if they are needed that badly I would think justifying them upstream would be doable...
Comment 15 Josh Stone 2012-09-07 16:15:39 EDT
(In reply to comment #14)
> (In reply to comment #13)
> > I prefer keeping the exports, because that's the main form of kernel
> > agreement that these functions are ok for us to use.  But we can get to them
> > anyway through kallsyms if necessary.
> 
> Not to be all grumpy, but "agreement" is kind of an overloaded word there. 
> If you're adding an EXPORT_SYMBOL (gpl or not) to the Fedora kernel but that
> isn't going to be sent or accepted upstream (per comment #7), that's kind of
> not agreement.
> 
> If you really need the exports then by all means keep them.  However, if
> they are needed that badly I would think justifying them upstream would be
> doable...

We've had no luck requesting exports for SystemTap's sake - to a few kernel folks that is even an anti-reason. :/  Upstream really wants exports to be justified by in-tree module use.  The generated nature of stap modules precludes going upstream, unless we got the whole darn tool in-tree like perf.  Not likely.

Kprobes is exported; it seems perfectly reasonable to me that uprobes' supporting functions should be exported too.  I don't know how to best argue for that though.

So for now I guess I meant that "agreement" is with you Fedora kernel maintainers, that stap is a legitimate user of these functions.  That may just mean that uprobes bugs discovered via stap will not be dismissed.  We can do that in a softer form via kallsyms though, if you prefer.
Comment 16 Anton Arapov 2012-09-08 13:12:30 EDT
I will take care of this one during the next uprobes rebase.
Comment 17 Anton Arapov 2012-09-12 04:51:47 EDT
fyi, change below will fix the issue, I will send it to fedora-kernel a bit later.

==
 kernel/events/uprobes.c | 3 +++
 kernel/ptrace.c         | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 336f069..f64156d 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -27,6 +27,7 @@
 #include <linux/pagemap.h>     /* read_mapping_page */
 #include <linux/slab.h>
 #include <linux/sched.h>
+#include <linux/export.h>
 #include <linux/rmap.h>                /* anon_vma_prepare */
 #include <linux/mmu_notifier.h>        /* set_pte_at_notify */
 #include <linux/swap.h>                /* try_to_free_swap */
@@ -902,6 +903,7 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *

       	return ret;
 }
+EXPORT_SYMBOL_GPL(uprobe_register);

 /*
  * uprobe_unregister - unregister a already registered probe.
@@ -933,6 +935,7 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume
       	if (uprobe)
               	put_uprobe(uprobe);
 }
+EXPORT_SYMBOL_GPL(uprobe_unregister);

 static struct rb_node *
 find_node_in_range(struct inode *inode, loff_t min, loff_t max)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index a232bb5..764fcd1 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -33,6 +33,12 @@ static int ptrace_trapping_sleep_fn(void *flags)
 }

 /*
+ * This is declared in linux/regset.h and defined in machine-dependent
+ * code.  We put the export here to ensure no machine forgets it.
+ */
+EXPORT_SYMBOL_GPL(task_user_regset_view);
+
+/*
  * ptrace a task: make the debugger its new parent and
  * move it to the ptrace list.
  *
Comment 19 Josh Boyer 2012-10-10 11:12:06 EDT
Fairly sure this is fixed.

I would like someone submit the symbol export patch upstream though.

Note You need to log in before you can comment on or make changes to this bug.