lkml.org 
[lkml]   [2018]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [kbuild-all] [PATCH] OPTIONAL: cpufreq/intel_pstate: fix debugfs_simple_attr.cocci warnings
    Date
    Julia Lawall <julia.lawall@lip6.fr> writes:

    > On Fri, 30 Mar 2018, Nicolai Stange wrote:
    >
    >> Julia Lawall <julia.lawall@lip6.fr> writes:
    >>
    >> > On Thu, 29 Mar 2018, Fabio Estevam wrote:
    >> >
    >> >> Hi Julia,
    >> >>
    >> >> On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
    >> >> > Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE
    >> >> > for debugfs files.
    >> >> >
    >> >> > Semantic patch information:
    >> >> > Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
    >> >> > imposes some significant overhead as compared to
    >> >> > DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().
    >> >>
    >> >> Just curious: could you please expand on what "imposes some
    >> >> significant overhead" means?
    >> >
    >> > I don't know. I didn't write this rule. Nicolai, can you explain?
    >>
    >> From commit 49d200deaa68 ("debugfs: prevent access to removed files' private
    >> data"):
    >>
    >> Upon return of debugfs_remove()/debugfs_remove_recursive(), it might
    >> still be attempted to access associated private file data through
    >> previously opened struct file objects. If that data has been freed by
    >> the caller of debugfs_remove*() in the meanwhile, the reading/writing
    >> process would either encounter a fault or, if the memory address in
    >> question has been reassigned again, unrelated data structures could get
    >> overwritten.
    >> [...]
    >> Currently, there are ~1000 call sites of debugfs_create_file() spread
    >> throughout the whole tree and touching all of those struct file_operations
    >> in order to make them file removal aware by means of checking the result of
    >> debugfs_use_file_start() from within their methods is unfeasible.
    >>
    >> Instead, wrap the struct file_operations by a lifetime managing proxy at
    >> file open [...]
    >>
    >> The additional overhead comes in terms of additional memory needed: for
    >> debugs files created through debugfs_create_file(), one such struct
    >> file_operations proxy is allocated for each struct file instantiation,
    >> c.f. full_proxy_open().
    >>
    >> This was needed to "repair" the ~1000 call sites without touching them.
    >>
    >> New debugfs users should make their file_operations removal aware
    >> themselves by means of DEFINE_DEBUGFS_ATTRIBUTE() and signal that fact to
    >> the debugfs core by instantiating them through
    >> debugfs_create_file_unsafe().
    >>
    >> See commit c64688081490 ("debugfs: add support for self-protecting
    >> attribute file fops") for further information.
    >
    > Thanks. Perhaps it would be good to add a reference to this commit in
    > the message generated by the semantic patch.

    Thanks for doing this!


    >
    > Would it be sufficient to just apply the semantic patch everywhere and
    > submit the patches?

    In principle yes. But I'm note sure whether such a mass application is
    worth it: the proxy allocation happens only at file open and the
    expectation is that there aren't that many debugfs files kept open at a
    time. OTOH, a struct file_operation consumes 256 bytes with
    sizeof(long) == 8.

    In my opinion, new users should avoid this overhead as it's easily
    doable. For existing ones, I don't know.

    Thanks,

    Nicolai

    --
    SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton,
    HRB 21284 (AG Nürnberg)

    \
     
     \ /
      Last update: 2018-03-31 06:21    [W:3.900 / U:0.560 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site