Prev: procfs: Kill BKL in llseek on proc base
Next: net/wireless/libertas: do not call wiphy_unregister() w/o wiphy_register()
From: Alexey Dobriyan on 30 Mar 2010 02:50
On Tue, Mar 30, 2010 at 9:20 AM, Frederic Weisbecker <fweisbec(a)gmail.com> wrote:
> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -231,9 +231,9 @@ static long proc_reg_unlocked_ioctl(struct file *file, unsigned int cmd, unsigne
> � � � � � � � �if (rv == -ENOIOCTLCMD)
> � � � � � � � � � � � �rv = -EINVAL;
> � � � �} else if (ioctl) {
> - � � � � � � � lock_kernel();
> + � � � � � � � WARN_ONCE(1, "Procfs ioctl handlers must use unlocked_ioctl, "
> + � � � � � � � � � � � � "%pf will be called without the Bkl held\n", ioctl);
> � � � � � � � �rv = ioctl(file->f_path.dentry->d_inode, file, cmd, arg);
> - � � � � � � � unlock_kernel();

Then delete the branch.
Or go through formal feature-removal procedure.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 30 Mar 2010 03:10
On Tue, Mar 30, 2010 at 09:38:11AM +0300, Alexey Dobriyan wrote:
> On Tue, Mar 30, 2010 at 9:20 AM, Frederic Weisbecker <fweisbec(a)gmail.com> wrote:
> > --- a/fs/proc/inode.c
> > +++ b/fs/proc/inode.c
> > @@ -231,9 +231,9 @@ static long proc_reg_unlocked_ioctl(struct file *file, unsigned int cmd, unsigne
> > � � � � � � � �if (rv == -ENOIOCTLCMD)
> > � � � � � � � � � � � �rv = -EINVAL;
> > � � � �} else if (ioctl) {
> > - � � � � � � � lock_kernel();
> > + � � � � � � � WARN_ONCE(1, "Procfs ioctl handlers must use unlocked_ioctl, "
> > + � � � � � � � � � � � � "%pf will be called without the Bkl held\n", ioctl);
> > � � � � � � � �rv = ioctl(file->f_path.dentry->d_inode, file, cmd, arg);
> > - � � � � � � � unlock_kernel();
>
> Then delete the branch.
> Or go through formal feature-removal procedure.


I thought about it. I even started to write something in this
feature-removal file but realized that I can't remove the
..ioctl() callback from file operations. We still need to check
the user hasn't made the mistake of implementing it.

What I can plan as a feature removal, though, is to keep the warning
but don't actually call the ioctl.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 30 Mar 2010 06:40
On Tuesday 30 March 2010, Frederic Weisbecker wrote:
> On Tue, Mar 30, 2010 at 09:38:11AM +0300, Alexey Dobriyan wrote:
> > On Tue, Mar 30, 2010 at 9:20 AM, Frederic Weisbecker <fweisbec(a)gmail.com> wrote:
> > > --- a/fs/proc/inode.c
> > > +++ b/fs/proc/inode.c
> > > @@ -231,9 +231,9 @@ static long proc_reg_unlocked_ioctl(struct file *file, unsigned int cmd, unsigne
> > > if (rv == -ENOIOCTLCMD)
> > > rv = -EINVAL;
> > > } else if (ioctl) {
> > > - lock_kernel();
> > > + WARN_ONCE(1, "Procfs ioctl handlers must use unlocked_ioctl, "
> > > + "%pf will be called without the Bkl held\n", ioctl);
> > > rv = ioctl(file->f_path.dentry->d_inode, file, cmd, arg);
> > > - unlock_kernel();
> >
> > Then delete the branch.
> > Or go through formal feature-removal procedure.
>
>
> I thought about it. I even started to write something in this
> feature-removal file but realized that I can't remove the
> .ioctl() callback from file operations. We still need to check
> the user hasn't made the mistake of implementing it.
>
> What I can plan as a feature removal, though, is to keep the warning
> but don't actually call the ioctl.

I believe we can actually remove ioctl from file_operations. The patch I did
to convert all users to ".unlocked_ioctl = default_ioctl," should really catch
all cases, and I think we can enforce this by renaming fops->ioctl to locked_ioctl
or old_ioctl to make sure we didn't miss any, and then mandate that this one
is only used when unlocked_ioctl is set to default_ioctl.

I also remember going through procfs ioctl operations some time ago and finding
exactly three users, which I believe are the same ones that Frederic found.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 31 Mar 2010 13:30
On Tue, Mar 30, 2010 at 11:33:40AM +0100, Arnd Bergmann wrote:
> On Tuesday 30 March 2010, Frederic Weisbecker wrote:
> > On Tue, Mar 30, 2010 at 09:38:11AM +0300, Alexey Dobriyan wrote:
> > > On Tue, Mar 30, 2010 at 9:20 AM, Frederic Weisbecker <fweisbec(a)gmail.com> wrote:
> > > > --- a/fs/proc/inode.c
> > > > +++ b/fs/proc/inode.c
> > > > @@ -231,9 +231,9 @@ static long proc_reg_unlocked_ioctl(struct file *file, unsigned int cmd, unsigne
> > > > if (rv == -ENOIOCTLCMD)
> > > > rv = -EINVAL;
> > > > } else if (ioctl) {
> > > > - lock_kernel();
> > > > + WARN_ONCE(1, "Procfs ioctl handlers must use unlocked_ioctl, "
> > > > + "%pf will be called without the Bkl held\n", ioctl);
> > > > rv = ioctl(file->f_path.dentry->d_inode, file, cmd, arg);
> > > > - unlock_kernel();
> > >
> > > Then delete the branch.
> > > Or go through formal feature-removal procedure.
> >
> >
> > I thought about it. I even started to write something in this
> > feature-removal file but realized that I can't remove the
> > .ioctl() callback from file operations. We still need to check
> > the user hasn't made the mistake of implementing it.
> >
> > What I can plan as a feature removal, though, is to keep the warning
> > but don't actually call the ioctl.
>
> I believe we can actually remove ioctl from file_operations. The patch I did
> to convert all users to ".unlocked_ioctl = default_ioctl," should really catch
> all cases, and I think we can enforce this by renaming fops->ioctl to locked_ioctl
> or old_ioctl to make sure we didn't miss any, and then mandate that this one
> is only used when unlocked_ioctl is set to default_ioctl.


I just looked at the patch in question and noted that the changelog
is pretty high, but how could it be else.
Actually it's not that large, but highly spread:

Documentation/DocBook/kernel-hacking.tmpl | 2 +-
Documentation/filesystems/vfs.txt | 3 +-
arch/arm/kernel/etm.c | 1 +
arch/cris/arch-v10/drivers/ds1302.c | 3 ++
arch/cris/arch-v10/drivers/gpio.c | 2 +
arch/cris/arch-v10/drivers/i2c.c | 2 +
arch/cris/arch-v10/drivers/pcf8563.c | 3 ++
arch/cris/arch-v10/drivers/sync_serial.c | 4 ++-
arch/cris/arch-v32/drivers/cryptocop.c | 4 ++-
arch/cris/arch-v32/drivers/i2c.c | 2 +
arch/cris/arch-v32/drivers/mach-a3/gpio.c | 2 +
arch/cris/arch-v32/drivers/mach-fs/gpio.c | 2 +
arch/cris/arch-v32/drivers/pcf8563.c | 5 +++-
arch/cris/arch-v32/drivers/sync_serial.c | 4 ++-
arch/ia64/kernel/perfmon.c | 2 +
arch/ia64/sn/kernel/sn2/sn_hwperf.c | 2 +
arch/m68k/bvme6000/rtc.c | 2 +
arch/m68k/mvme16x/rtc.c | 2 +
arch/um/drivers/harddog_kern.c | 2 +
arch/um/drivers/hostaudio_kern.c | 3 ++
arch/um/drivers/mmapper_kern.c | 3 ++
drivers/block/DAC960.c | 3 +-
drivers/block/paride/pg.c | 2 +
drivers/block/paride/pt.c | 2 +
drivers/block/pktcdvd.c | 3 ++
drivers/char/apm-emulation.c | 2 +
drivers/char/applicom.c | 2 +
drivers/char/ds1302.c | 1 +
drivers/char/ds1620.c | 2 +
drivers/char/dtlk.c | 2 +
drivers/char/generic_nvram.c | 2 +
drivers/char/genrtc.c | 2 +
drivers/char/i8k.c | 2 +
drivers/char/ip2/ip2main.c | 1 +
drivers/char/ipmi/ipmi_devintf.c | 2 +
drivers/char/ipmi/ipmi_watchdog.c | 2 +
drivers/char/istallion.c | 1 +
drivers/char/lp.c | 1 +
drivers/char/mmtimer.c | 1 +
drivers/char/nwflash.c | 1 +
drivers/char/raw.c | 4 +++
drivers/char/rio/rio_linux.c | 1 +
drivers/char/stallion.c | 1 +
drivers/char/sx.c | 1 +
drivers/char/uv_mmtimer.c | 1 +
drivers/char/viotape.c | 1 +
drivers/firewire/core-cdev.c | 2 +
drivers/gpu/drm/i810/i810_dma.c | 2 +
drivers/gpu/drm/i830/i830_dma.c | 2 +
drivers/hid/usbhid/hiddev.c | 3 +-
drivers/hwmon/fschmd.c | 2 +
drivers/ide/ide-tape.c | 2 +
drivers/ieee1394/dv1394.c | 2 +
drivers/ieee1394/raw1394.c | 2 +
drivers/ieee1394/video1394.c | 4 ++-
drivers/infiniband/core/ucm.c | 2 +
drivers/infiniband/core/ucma.c | 2 +
drivers/infiniband/core/user_mad.c | 7 +++-
drivers/infiniband/core/uverbs_main.c | 10 +++++--
drivers/input/misc/hp_sdc_rtc.c | 2 +
drivers/input/misc/uinput.c | 1 +
drivers/isdn/capi/capi.c | 1 +
drivers/isdn/divert/divert_procfs.c | 2 +
drivers/isdn/i4l/isdn_common.c | 1 +
drivers/isdn/mISDN/timerdev.c | 3 ++
drivers/macintosh/ans-lcd.c | 2 +
drivers/macintosh/nvram.c | 2 +
drivers/macintosh/via-pmu.c | 2 +
drivers/media/dvb/bt8xx/dst_ca.c | 1 +
drivers/media/dvb/dvb-core/dmxdev.c | 3 ++
drivers/media/dvb/dvb-core/dvb_ca_en50221.c | 3 ++
drivers/media/dvb/dvb-core/dvb_frontend.c | 5 +++-
drivers/media/dvb/dvb-core/dvb_net.c | 3 ++
drivers/media/dvb/firewire/firedtv-ci.c | 3 ++
drivers/media/dvb/ttpci/av7110.c | 3 ++
drivers/media/dvb/ttpci/av7110_av.c | 5 +++
drivers/media/dvb/ttpci/av7110_ca.c | 3 ++
drivers/media/video/cpia.c | 2 +
drivers/media/video/v4l2-dev.c | 2 +
drivers/mtd/mtdchar.c | 1 +
drivers/mtd/ubi/cdev.c | 2 +
drivers/net/ppp_generic.c | 4 ++-
drivers/net/wireless/airo.c | 9 ++++++
drivers/net/wireless/ray_cs.c | 3 ++
drivers/rtc/rtc-m41t80.c | 1 +
drivers/s390/char/fs3270.c | 1 +
drivers/s390/char/tape_char.c | 2 +-
drivers/s390/cio/chsc_sch.c | 2 +
drivers/s390/crypto/zcrypt_api.c | 1 +
drivers/s390/scsi/zfcp_cfdc.c | 2 +
drivers/sbus/char/envctrl.c | 1 +
drivers/sbus/char/openprom.c | 1 +
drivers/scsi/3w-9xxx.c | 2 +
drivers/scsi/3w-sas.c | 2 +
drivers/scsi/3w-xxxx.c | 2 +
drivers/scsi/aacraid/linit.c | 1 +
drivers/scsi/dpt_i2o.c | 2 +
drivers/scsi/gdth.c | 2 +
drivers/scsi/megaraid.c | 2 +
drivers/scsi/megaraid/megaraid_mm.c | 2 +
drivers/scsi/megaraid/megaraid_sas.c | 1 +
drivers/scsi/mpt2sas/mpt2sas_ctl.c | 1 +
drivers/scsi/osd/osd_uld.c | 2 +
drivers/scsi/osst.c | 2 +
drivers/scsi/pmcraid.c | 2 +
drivers/scsi/sg.c | 2 +
drivers/scsi/st.c | 1 +
drivers/spi/spidev.c | 2 +
drivers/staging/b3dfg/b3dfg.c | 2 +
drivers/staging/comedi/comedi_fops.c | 2 +
drivers/staging/dream/pmem.c | 3 ++
drivers/staging/dream/qdsp5/audio_aac.c | 2 +
drivers/staging/dream/qdsp5/audio_mp3.c | 2 +
drivers/staging/poch/poch.c | 3 ++
drivers/staging/sep/sep_driver.c | 3 ++
drivers/staging/vme/devices/vme_user.c | 2 +
drivers/telephony/ixj.c | 1 +
drivers/usb/class/usblp.c | 2 +
drivers/usb/gadget/printer.c | 1 +
drivers/usb/misc/idmouse.c | 2 +
drivers/usb/misc/iowarrior.c | 1 +
drivers/usb/misc/rio500.c | 1 +
drivers/usb/misc/vstusb.c | 2 +
fs/autofs/root.c | 1 +
fs/autofs4/dev-ioctl.c | 2 +
fs/btrfs/super.c | 1 +
fs/coda/pioctl.c | 3 ++
fs/coda/psdev.c | 2 +
fs/ecryptfs/file.c | 2 +
fs/ecryptfs/miscdev.c | 2 +
fs/hfsplus/dir.c | 2 +
fs/hfsplus/inode.c | 2 +
fs/ioctl.c | 11 ++------
fs/ncpfs/dir.c | 2 +
fs/ncpfs/file.c | 1 +
fs/read_write.c | 34 ------------------------
fs/smbfs/dir.c | 2 +
fs/smbfs/file.c | 1 +
fs/udf/dir.c | 2 +
fs/udf/file.c | 1 +
include/linux/fs.h | 5 ---
include/linux/smp_lock.h | 5 +++
lib/kernel_lock.c | 37 +++++++++++++++++++++++++-
net/socket.c | 1 +
sound/core/control.c | 2 +
sound/core/oss/pcm_oss.c | 2 +
sound/core/pcm_native.c | 2 +
sound/core/seq/seq_clientmgr.c | 2 +
sound/oss/au1550_ac97.c | 30 +++++++++++----------
sound/oss/dmasound/dmasound_core.c | 2 +
sound/oss/msnd_pinnacle.c | 2 +
sound/oss/sh_dac_audio.c | 3 ++
sound/oss/soundcard.c | 1 +
sound/oss/swarm_cs4297a.c | 3 ++
sound/oss/vwsnd.c | 2 +
sound/soc/soc-core.c | 2 +
virt/kvm/kvm_main.c | 4 +++
157 files changed, 372 insertions(+), 80 deletions(-)


I wonder if we should actually just turn all these into unlocked_ioctl
directly. And then bring a warn on ioctl, and finally schedule the removal
of this callback.

What do you think?

You plan looks good but I fear this actually carries the problem forward
in that we won't be able to remove .ioctl after that.

I can handle that if you agree.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 31 Mar 2010 16:30
On Wednesday 31 March 2010 19:22:11 Frederic Weisbecker wrote:
> On Tue, Mar 30, 2010 at 11:33:40AM +0100, Arnd Bergmann wrote:
> > I believe we can actually remove ioctl from file_operations. The patch I did
> > to convert all users to ".unlocked_ioctl = default_ioctl," should really catch
> > all cases, and I think we can enforce this by renaming fops->ioctl to locked_ioctl
> > or old_ioctl to make sure we didn't miss any, and then mandate that this one
> > is only used when unlocked_ioctl is set to default_ioctl.
>
> I just looked at the patch in question and noted that the changelog
> is pretty high, but how could it be else.
> Actually it's not that large, but highly spread:
<snip>
> 157 files changed, 372 insertions(+), 80 deletions(-)
>
>
> I wonder if we should actually just turn all these into unlocked_ioctl
> directly. And then bring a warn on ioctl, and finally schedule the removal
> of this callback.
>
> What do you think?

I don't think the warning helps all that much, at least not across an
entire release. We could leave it in for the merge window and fix all
users for -rc1, then submit a patch that kills everything that came
in during the merge window and remove it completely in -rc2.

Getting rid of ioctl completely is a lot of work though, covering the
entire lot of ~150 device drivers. I think the patch as is (or the
variant renaming .ioctl to .locked_ioctl) is far less work and has
less potential of introducing regressions.

> You plan looks good but I fear this actually carries the problem forward
> in that we won't be able to remove .ioctl after that.
>
> I can handle that if you agree.

I don't think we really need to get rid of it this soon in the obsolete
drivers, pushing down the BKL into an unlocked_ioctl function only slightly
shifts the problem around, since the driver still depends on the BKL then
and gets disabled if you build with CONFIG_BKL=n.

IMHO, a better use of your time would be to completely remove the BKL
along with the ioctl function from any of the drivers in this lists that
looks like it could be relevant to real users.

In the meantime, we can move the declaration of the .locked_ioctl callback
into an #ifdef CONFIG_BKL, to make sure nobody builds a driver with an
ioctl function that does not get called.

Another crazy idea I had was to simply turn the BKL into a regular mutex
as soon as we can show that all remaining users are of the non-recursive
kind and don't rely on the autorelease-on-sleep. Doing that would be
much easier without the pushdown into .unlocked_ioctl than it would be
with it.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2 3 4 5 6
Prev: procfs: Kill BKL in llseek on proc base
Next: net/wireless/libertas: do not call wiphy_unregister() w/o wiphy_register()