Bug 849364

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.

(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.

(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

(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.

(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 ;)

(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?

(In reply to comment #6)
> 
> Is that going upstream soon?

I guess no.

(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.

(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.

(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.

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.

(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.

(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.

(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...

(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.

I will take care of this one during the next uprobes rebase.

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.
  *

http://lists.fedoraproject.org/pipermail/kernel/2012-September/003947.html

Fairly sure this is fixed.

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