netdev - Re: [PATCH] wil6210: fix debugfs_simple

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.21.1810061422030.2363@hadrien>
Date:   Sat, 6 Oct 2018 14:22:58 +0200 (CEST)
From:   Julia Lawall <julia.lawall@...6.fr>
To:     Kalle Valo <kvalo@...eaurora.org>
cc:     Julia Lawall <julia.lawall@...6.fr>,
        YueHaibing <yuehaibing@...wei.com>,
        Maya Erez <merez@...eaurora.org>,
        linux-wireless@...r.kernel.org, wil6210@....qualcomm.com,
        kernel-janitors@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] wil6210: fix debugfs_simple_attr.cocci warnings



On Sat, 6 Oct 2018, Kalle Valo wrote:

> Julia Lawall <julia.lawall@...6.fr> writes:
>
> > On Fri, 5 Oct 2018, Kalle Valo wrote:
> >
> >> YueHaibing <yuehaibing@...wei.com> writes:
> >>
> >> > 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().
> >> >
> >> > Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
> >>
> >> Just out of curiosity, what kind of overhead are we talking about here?
> >
> > The log message on the commit introducing the semantic patch says the
> > following:
> >
> >     In order to protect against file removal races, debugfs files created via
> >     debugfs_create_file() now get wrapped by a struct file_operations at their
> >     opening.
> >
> >     If the original struct file_operations are known to be safe against removal
> >     races by themselves already, the proxy creation may be bypassed by creating
> >     the files through debugfs_create_file_unsafe().
> >
> >     In order to help debugfs users who use the common
> >       DEFINE_SIMPLE_ATTRIBUTE() + debugfs_create_file()
> >     idiom to transition to removal safe struct file_operations, the helper
> >     macro DEFINE_DEBUGFS_ATTRIBUTE() has been introduced.
> >
> >     Thus, the preferred strategy is to use
> >       DEFINE_DEBUGFS_ATTRIBUTE() + debugfs_create_file_unsafe()
> >     now.
>
> I admit that I didn't have time to investigate this is detail but I'm
> still not understanding where is that "significant overhead" coming from
> and how big of overhead are we talking about? I guess it has something
> to do with full_proxy_open() vs open_proxy_open()?
>
> Not that I'm against this patch, just curious when I see someone
> claiming "significant overhead" which is not obvious for me.

The message with the semantic patch doesn't really talk about significant
overhead.  Maybe YueHaibing can discuss with the person who proposed the
semantic patch what the actual issue is, and when the proposed change is
actually applicable.

julia


>
> --
> Kalle Valo
>