linux-kernel - Re: [PATCH] kernel/params.c: make use of unused but set variable

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]

Date:	Thu, 11 Jun 2015 10:54:27 +0900
From:	Tejun Heo <htejun@...il.com>
To:	Louis Langholtz <lou_langholtz@...com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	trivial@...nel.org, Rusty Russell <rusty@...tcorp.com.au>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] kernel/params.c: make use of unused but set variable

Hey, Louis.

On Wed, Jun 10, 2015 at 11:05:21AM -0600, Louis Langholtz wrote:
> The underlying code for sysfs_create_file does call WARN to warn about
> any errors. So it's not like the code is totally silent anyway. Then the unused

Not any errors.  It triggers warning on missing ops and dup file
names.  The former is a pretty fundamental usage error and we've had
too many of the latter unhandled despite of __must_check.

> but set variable in params.c (that's set to its return value) can be removed
> (and the compiler warning resolved). Incidentally, I do think it'd be helpful
> to comment document this behavior of using WARN so that callers can
> more readily recognize (and be assured that) they won't need to call WARN
> (unless callers want to add __FILE__ and __LINE__ info to the printk output).
> 
> Having functions marked __must_check seems to make more sense when
> their return values are *always* necessary for calling code to have any
> business calling them. Like when there's one or more future calls to functions
> that have to be made (or not made) for the given resource (the kobject) that
> depend on that function's return value (such that the return value state is
> always a later conditional). That doesn't appear to be the case however for
> sysfs_create_file().

I don't know.  Sounds like a weird rule.  We had __must_check, still
had certain error types unhandled so people added explicit WARN to
force people to look into those issues and that means __must_check
should go away?  Sure, if it warns on all errors and can return void
then there's nothing to discuss but that's not the case.  Here, an
error return indicates userland visible behavior difference.  I'd
venture to say that the return value is pretty darn important.

...
> Adding __must_check probably made it easier for developers to identify calling
> code that was depending on success from these functions. At the same time,
> it's not true (at least currently) that these return values always need to

I don't know.  The "always" rule you're speaking of seems too
arbitrary and rigid to me.  Always is a tricky word and tying oneself
to things like that usually doesn't lead to healthy trade-offs.

I don't think removing __must_check would be a disaster but at the
same time these terminal sysfs functions look like the perfect
candidates for such annotations where error returns indicate subtle
but visible behavior differences visible to userland while not
affecting in-kernel operations at all and thus can be easily ignored.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/