linux-kernel - Re: [PATCH 24/61] sysfs: make sysfs

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:	Fri, 13 Jul 2007 01:16:35 +0530
From:	"Satyam Sharma" <satyam.sharma@...il.com>
To:	"Tejun Heo" <htejun@...il.com>
Cc:	"YOSHIFUJI Hideaki / 吉藤英明" 
	<yoshfuji@...ux-ipv6.org>, gregkh@...e.de,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 24/61] sysfs: make sysfs_put() ignore NULL sd

Hi,

> >>>> Make sysfs_put() ignore NULL sd instead of oopsing.
> >>> I do not think this is a good idea; it is non-sense (and rather a bug)
> >>> to call "put" with NULL argument in general.
> >> It's better than having to check it all the time in the caller :)
> >
> > How many callers do we have that will get benefit from this change?
> >
> > Well, the change will hide the bug. It seems all callers in fs/sysfs
> > already assume that the argument is NOT NULL, and it is a bug to call
> > sysfs_put() with NULL; the function should be used to "put" something
> > you "have" (non-NULL). If it is called with NULL, I would say, we
> > should BUG here to detect the logical bug.

Yoshifuji is 100% correct, IMNSHO.

Please, this is _basic_ refcounting semantics. For those who disagree,
kindly read Yoshifuji's above paragraph again.

> Well, I'm okay either way.  It's not like one way is undisputably better
> than the other

Yes, it is, of course. Allowing xxx_put(NULL) to succeed (without any
warnings/oops) is *absolutely* nonsensical, and can *only* occur if the
caller (or worse, the API itself) is buggy in the first place (i.e. does not
use proper locking and/or refcounting).

I can't believe it should be so difficult to understand this. How can any
caller (that first did a xxx_get() on that shared object) land up with that
object getting NULL _from under it_ unless some logic is wrong
somewhere? And instead of flagging this broken logic, the proposed
change here would hide it.

Worse, if that object did become NULL between the _get() and _put()
code, then we'll have an oops (which would be even more difficult to
debug now) anyway.

> but we're leaning toward accepting NULL argument in this
> type of functions.  Think about kfree(NULL) and its usefulness.

Don't {mis}quote the kfree() mistake here, please.

> More
> importantly, the ecosystem around sysfs - that is, kobject, driver model
> - generally accepts NULL argument for their get/put functions

This can only mean two things:

(1) Either, they simply do not _need_ the refcounting in the first place
(which means -- better do away with get() and put() for them altogether)

(2) Or, all that code / APIs are so horribly misdesigned and/or buggy that
you're now having to hide that by allowing NULL arguments in get() and
put() functions (which means -- fix the bugs, please)

> so unless
> there's a compelling reason to convert them all, and I don't see any,
> sysfs_put() needs to follow the same rule.

Ok, what's the compelling reason to change sysfs_put() then?
I don't see any, either.

Satyam
-
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/