Re: [PATCH 6/6] procfs: Kill the bkl in ioctl

From: Frederic Weisbecker
Date: Wed Mar 31 2010 - 13:29:04 EST

Next message: Greg KH: "Re: [PATCH] drivers:staging: sources for ST core"
Previous message: San Mehat: "[PATCH] proc: pagemap: Hold mmap_sem during page walk"
In reply to: Arnd Bergmann: "Re: [PATCH 6/6] procfs: Kill the bkl in ioctl"
Next in thread: Arnd Bergmann: "Re: [PATCH 6/6] procfs: Kill the bkl in ioctl"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Greg KH: "Re: [PATCH] drivers:staging: sources for ST core"
Previous message: San Mehat: "[PATCH] proc: pagemap: Hold mmap_sem during page walk"
In reply to: Arnd Bergmann: "Re: [PATCH 6/6] procfs: Kill the bkl in ioctl"
Next in thread: Arnd Bergmann: "Re: [PATCH 6/6] procfs: Kill the bkl in ioctl"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]