linux-kernel - Re: [PATCH 2.6.24] block2mtd: removing a device and typo fixes

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: <20080219150822.GA29587@lazybastard.org>
Date:	Tue, 19 Feb 2008 16:08:22 +0100
From:	Jörn Engel <joern@...fs.org>
To:	Stephane Chazelas <stephane.chazelas@...rson.com>
Cc:	Jörn Engel <joern@...fs.org>,
	linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org,
	Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH 2.6.24] block2mtd: removing a device and typo fixes

[ Just returned home. ]

On Tue, 12 February 2008 16:10:34 +0000, Stephane Chazelas wrote:
> 
> OK, I can do that. Would the "simple" fix go to the
> trivial@...nel.org Trivial Patch Monkey?

That or to me or to dwmw2...  I'm not too picky about the path, as long
as it gets merged eventually.

> > The core of remove_device_by_name() is shared with block2mtd_exit(),
> > so a common helper would be good.  Your error handling is better, so
> > let's keep that version.
> 
> Well, yes that raised a concern to me, the "exit" function
> returns "void". If the del_mtd_device fails with EBUSY at the
> moment (such as when a /dev/mtdblock<x> is mounted), we ignore
> it and go on with freeing everything leaving a dangling mtd.
> 
> Is there a way around that?

For module unload we should be safe.  Errors are only returned if the
device doesn't exist (a clear bug) or if the device is still being used.
Module refcounting should prevent the latter case, so either way we have
a bug if del_mtd_device returns an error.

When removing the device via your proposed interface we should simply
return the error to userspace, if the interface permits.  Sysfs doesn't
seem to permit error returns, so we should keep the device alive and do
nothing.

> Another problem is that it's not easy to check whether a mtd
> creation was successful or not. Basically, you have to write to
> a /sys file and then parse /proc/mtd to get the result.

Agreed, sysfs' lack of error indication isn't ideal.

> What about having a /dev/block2mtd (with owner/permissions that
> could allow non-root users to use it), with 2 ioctls:
> 
> - one to "link" a block dev to a mtd that would take as
>   parameter a fd to an open block dev (again allowing for
>   flexible permissions) and would return the number of the
>   allocated mtd and success/failure in errno. Upon sucess it
>   would increase the refcnt of block2mtd.
> 
> - and one to "release" the link. That would fail if the mtd is
>   in use and decrease block2mtd's refcnt upon success.
> 
> A bit like the loop devices (or /dev/ptmx) actually. What do you
> think?

Could work.  Passing the fd raises several alarm bells.  Arnd, any
comments from you?

In general I'd like to see some good reasons for adding an ioctl (or any
other) interface first.  Creating interfaces is hard and you rarely get
a second chance to fix the mess you created before you knew all the
consequenses.

> (also, with the /sys interface, isn't there a potential problem
> with chroots wrt the path given to the /sys file?)

There may be.  Can you check if there is?

Jörn

-- 
Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it.
-- Brian W. Kernighan
--
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/