diff mbox

export symbol mount_lock

Message ID 855c5c7d-586a-3b35-76f8-ddca4fae1410@nasa.gov (mailing list archive)
State New, archived
Headers show

Commit Message

Jay Lan July 12, 2017, 9:40 p.m. UTC
Hi all,

I was writing a livepatch patch, but compilation failed in creating the 
.ko because mount_lock is "undefined."

The mount_lock is defined globally in fs/namespace.c
__cacheline_aligned_in_smp DEFINE_SEQLOCK(mount_lock);

and extern in fs/mount.h.
extern seqlock_t mount_lock;

If there is a reason that mount_lock should not be EXPORT_SYMBOL_GPL, 
please advise; otherwise, I propose the patch to export this symbol.

Signed-off-by: Jay Lan <jay.j.lan@nasa.gov>
---
  fs/namespace.c | 1 +
  1 file changed, 1 insertion(+)

  {

Comments

Christoph Hellwig July 13, 2017, 7:15 a.m. UTC | #1
On Wed, Jul 12, 2017 at 02:40:11PM -0700, Jay Lan wrote:
> Hi all,
> 
> I was writing a livepatch patch, but compilation failed in creating the .ko
> because mount_lock is "undefined."
> 
> The mount_lock is defined globally in fs/namespace.c
> __cacheline_aligned_in_smp DEFINE_SEQLOCK(mount_lock);
> 
> and extern in fs/mount.h.
> extern seqlock_t mount_lock;
> 
> If there is a reason that mount_lock should not be EXPORT_SYMBOL_GPL, please
> advise; otherwise, I propose the patch to export this symbol.

Because there is no %$^$^ reason to use it from a module ever?
Jay Lan July 13, 2017, 6:24 p.m. UTC | #2
On 07/13/2017 12:15 AM, Christoph Hellwig wrote:
> On Wed, Jul 12, 2017 at 02:40:11PM -0700, Jay Lan wrote:
>> Hi all,
>>
>> I was writing a livepatch patch, but compilation failed in creating the .ko
>> because mount_lock is "undefined."
>>
>> The mount_lock is defined globally in fs/namespace.c
>> __cacheline_aligned_in_smp DEFINE_SEQLOCK(mount_lock);
>>
>> and extern in fs/mount.h.
>> extern seqlock_t mount_lock;
>>
>> If there is a reason that mount_lock should not be EXPORT_SYMBOL_GPL, please
>> advise; otherwise, I propose the patch to export this symbol.
> Because there is no %$^$^ reason to use it from a module ever?
Is livepatch a good reason? Besides security fixes, livepatch is a good tool
to use for debugging and providing a temporary fix on production systems
(until next release from vendors) I know this argument is not strong, but at
least mount_lock is declared globally. Yes?

Thanks,
Jay
Matthew Wilcox July 13, 2017, 6:58 p.m. UTC | #3
On Thu, Jul 13, 2017 at 11:24:01AM -0700, Jay Lan wrote:
> On 07/13/2017 12:15 AM, Christoph Hellwig wrote:
> > On Wed, Jul 12, 2017 at 02:40:11PM -0700, Jay Lan wrote:
> > > Hi all,
> > > 
> > > I was writing a livepatch patch, but compilation failed in creating the .ko
> > > because mount_lock is "undefined."
> > > 
> > > The mount_lock is defined globally in fs/namespace.c
> > > __cacheline_aligned_in_smp DEFINE_SEQLOCK(mount_lock);
> > > 
> > > and extern in fs/mount.h.
> > > extern seqlock_t mount_lock;
> > > 
> > > If there is a reason that mount_lock should not be EXPORT_SYMBOL_GPL, please
> > > advise; otherwise, I propose the patch to export this symbol.
> > Because there is no %$^$^ reason to use it from a module ever?
> Is livepatch a good reason? Besides security fixes, livepatch is a good tool
> to use for debugging and providing a temporary fix on production systems
> (until next release from vendors) I know this argument is not strong, but at
> least mount_lock is declared globally. Yes?

Something I've done in the past as a local hack when a symbol I wanted
wasn't exported is:

extern seqlock_t *mount_lock_p;
module_param(mount_lock_p, charp, 0);

and then pass the address (from /proc/kallsyms) as a module parameter
at load time.
Jay Lan July 13, 2017, 7:09 p.m. UTC | #4
On 07/13/2017 11:58 AM, Matthew Wilcox wrote:
> On Thu, Jul 13, 2017 at 11:24:01AM -0700, Jay Lan wrote:
>> On 07/13/2017 12:15 AM, Christoph Hellwig wrote:
>>> On Wed, Jul 12, 2017 at 02:40:11PM -0700, Jay Lan wrote:
>>>> Hi all,
>>>>
>>>> I was writing a livepatch patch, but compilation failed in creating the .ko
>>>> because mount_lock is "undefined."
>>>>
>>>> The mount_lock is defined globally in fs/namespace.c
>>>> __cacheline_aligned_in_smp DEFINE_SEQLOCK(mount_lock);
>>>>
>>>> and extern in fs/mount.h.
>>>> extern seqlock_t mount_lock;
>>>>
>>>> If there is a reason that mount_lock should not be EXPORT_SYMBOL_GPL, please
>>>> advise; otherwise, I propose the patch to export this symbol.
>>> Because there is no %$^$^ reason to use it from a module ever?
>> Is livepatch a good reason? Besides security fixes, livepatch is a good tool
>> to use for debugging and providing a temporary fix on production systems
>> (until next release from vendors) I know this argument is not strong, but at
>> least mount_lock is declared globally. Yes?
> Something I've done in the past as a local hack when a symbol I wanted
> wasn't exported is:
>
> extern seqlock_t *mount_lock_p;
> module_param(mount_lock_p, charp, 0);
>
> and then pass the address (from /proc/kallsyms) as a module parameter
> at load time.

Thanks for the tip, Matthew!
Please considered the patch request withdrawn.

Jay
Al Viro July 14, 2017, 1:19 a.m. UTC | #5
On Thu, Jul 13, 2017 at 11:24:01AM -0700, Jay Lan wrote:
> On 07/13/2017 12:15 AM, Christoph Hellwig wrote:
> > On Wed, Jul 12, 2017 at 02:40:11PM -0700, Jay Lan wrote:
> > > Hi all,
> > > 
> > > I was writing a livepatch patch, but compilation failed in creating the .ko
> > > because mount_lock is "undefined."
> > > 
> > > The mount_lock is defined globally in fs/namespace.c
> > > __cacheline_aligned_in_smp DEFINE_SEQLOCK(mount_lock);
> > > 
> > > and extern in fs/mount.h.
> > > extern seqlock_t mount_lock;
> > > 
> > > If there is a reason that mount_lock should not be EXPORT_SYMBOL_GPL, please
> > > advise; otherwise, I propose the patch to export this symbol.
> > Because there is no %$^$^ reason to use it from a module ever?
> Is livepatch a good reason? Besides security fixes, livepatch is a good tool
> to use for debugging and providing a temporary fix on production systems
> (until next release from vendors) I know this argument is not strong, but at
> least mount_lock is declared globally. Yes?

No.  If you are playing with mount_lock, the odds are that you are accessing the
data structures declared in fs/mount.h.  They are not in include/* for a damn
good reason - we want to be able to change them at zero notice in any way we
bloody well like.  Without having to locate someone's code that stuck its private
parts into the machinery.

"Not static in file" != "everyone is welcome to play with that area".

Again, *ANY* module playing with the stuff in struct mount, let alone the
hashes, etc. is in position to play Charlie Chaplin's Modern Times stunt - the
one with travel through the gears.  At zero notice.  With no apologies for the
pain experienced by bereft users or for unkind replies to resulting complaints.

You Have Been Warned: DON'T GO THERE.

I don't know if I can make it any more clear short of running afoul of CDA and
its ilk.
diff mbox

Patch

diff --git a/fs/namespace.c b/fs/namespace.c
index 5a4438445bf7..3bbb619391ce 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -83,6 +83,7 @@  EXPORT_SYMBOL_GPL(fs_kobj);
   * tree or hash is modified or when a vfsmount structure is modified.
   */
  __cacheline_aligned_in_smp DEFINE_SEQLOCK(mount_lock);
+EXPORT_SYMBOL_GPL(mount_lock);

  static inline struct hlist_head *m_hash(struct vfsmount *mnt, struct 
dentry *dentry)