Skip to contentSkip to conversation

Linux 3.3-rc5

PATCH In review Linus Torvalds 4 participants 14 messages Feb 25, 2012 – Mar 2, 2012 Char and misc drivers

lore
Share & tools
lore
#0 lore raw

Hey, no delays this week.

And nothing really odd going on either. Maybe things are finally calming down.

Sure, I'd have liked it to be even calmer, and there's is movement in various areas: btrfs updates along with scsi and media driver updates. And various noise elsewhere. But on the whole it's been pretty boring, which is just how I like it.

A couple of the fixes were directly linked to my -rc4 announcement, where I told people they could use a 64-bit kernel to avoid the FP state save/restore problem we used to have. That brought up some fairly corner-case compatibility issues that could make that impractical. So hopefully it's *true* this time around.

And while the FP state save problem is gone, if you have a 64-bit capable CPU but are still running a 32-bit distro, it really would be interesting to verify that a 64-bit kernel works for you without problems. Because it always *should* have, but clearly that wasn't always the case. It would be very interesting to hear from people who have perhaps tried and failed before and perhaps didn't even bother to report the failure? Maybe it's worth trying again?

Linus

---
Al Viro (2):
vfs: fix compat_sys_stat() handling of overflows in st_nlink
ocfs2: deal with wraparounds of i_nlink in ocfs2_rename()

Alan Stern (2):
[SCSI] scsi_pm: Fix bug in the SCSI power management handler
usb-storage: fix freezing of the scanning thread

Alex Deucher (1):
drm/radeon/kms/atom: dpms bios scratch reg updates

Alexander Stein (2):
m68k: Add shared bit to Coldfire kernel page entries
m68k: Do not set global share for non-kernel shared pages

Alexey Khoroshilov (1):
[SCSI] mpt2sas: Fix mismatch in
mpt2sas_base_hard_reset_handler() mutex lock-unlock

Andrew Lunn (1):
USB: Serial: ti_usb_3410_5052: Add Abbot Diabetes Care cable id

Andrew Vasquez (4):
[SCSI] qla2xxx: Add an "is reset active" helper.
[SCSI] qla2xxx: Clear options-flags while issuing stop-firmware
mbx command.
[SCSI] qla2xxx: Remove errant clearing of MBX_INTERRUPT flag
during CT-IOCB processing.
[SCSI] qla2xxx: Correct out of bounds read of ISP2200 mailbox registers.

Anton Altaparmakov (1):
Restore direct_io / truncate locking API

Anton Vorontsov (1):
mm: memcg: Correct unregistring of events attached to the same eventfd

Arne Jansen (1):
btrfs: don't check DUP chunks twice

Arun Easi (1):
[SCSI] qla2xxx: Propagate up abort failures.

Ben Hutchings (1):
builddeb: Don't create files in /tmp with predictable names

Benjamin Herrenschmidt (2):
cpuidle: Default y on powerpc pSeries
powerpc: Fix various issues with return to userspace

Bruno Thomsen (1):

      USB: Added Kamstrup VID/PIDs to cp210x serial driver.

Catalin Marinas (1):
ARM: 7323/1: Do not allow ARM_LPAE on pre-ARMv7 architectures

Chad Dupuis (2):
[SCSI] qla2xxx: Add check for null fcport references in
qla2xxx_queuecommand.

      [SCSI] qla2xxx: Update version number to 8.03.07.13-k.

Chris D Schimp (2):
hwmon: (max6639) Fix FAN_FROM_REG calculation
hwmon: (max6639) Fix PPR register initialization to set both channels

Chris Mason (6):
Btrfs: don't reserve data with extents locked in btrfs_fallocate
Btrfs: improve error handling for btrfs_insert_dir_item callers
Btrfs: make sure we update latest_bdev
Btrfs: add extra sanity checks on the path names in btrfs_mksubvol
Btrfs: clear the extent uptodate bits during parent transid failures
Btrfs: fix compiler warnings on 32 bit systems

Danny Kukawka (1):
arch/arm/mach-shmobile/board-ag5evm.c: included linux/dma-mapping.h twice

Dave Jiang (1):
[SCSI] isci: Fix NULL ptr dereference when no firmware is being loaded

David Howells (2):
NOMMU: Lock i_mmap_mutex for access to the VMA prio list
NOMMU: Don't need to clear vm_mm when deleting a VMA

David Sterba (2):
btrfs: fix structs where bitfields and spinlock/atomic share 8B word
btrfs: silence warning in raid array setup

Dimitri Sivanich (1):
vfs: fix panic in __d_lookup() with high dentry hashtable counts

Dmitry Kasatkin (1):
digsig: changed type of the timestamp

Doug Ledford (1):
mlx4_core: Exported functions can't be static

Elric Fu (1):
USB: Set hub depth after USB3 hub reset

Eric Paris (1):
ARM/audit: include audit header and fix audit arch

Eugeni Dodonov (4):
drm/i915: gen7: implement rczunit workaround drm/i915: gen7: Implement an L3 caching workaround. drm/i915: gen7: work around a system hang on IVB drm/i915: do not enable RC6p on Sandy Bridge

Florian Albrechtskirchinger (1):
btrfs: honor umask when creating subvol root

Giridhar Malavali (2):
[SCSI] qla2xxx: Complete mailbox command timedout to avoid
initialization failures during next reset cycle.
[SCSI] qla2xxx: Proper detection of firmware abort error code for ISP82xx.

Greg Dietsche (1):
coccicheck: change handling of C={1,2} when M= is set

Greg Ungerer (1):
m68knommu: fix syscall tracing stuck process

Guennadi Liakhovetski (4):
ARM: mach-shmobile: simplify MMCIF DMA configuration
sh: sh7757lcr: update to the new MMCIF DMA configuration
ARM: mach-shmobile: add GPIO-to-IRQ translation to sh7372
sh: se7724: fix compile breakage

Guenter Roeck (2):
hwmon: (ads1015) Fix file leak in probe function
hwmon: (max34440) Fix resetting temperature history

Heiko Carstens (1):
[S390] idle: avoid RCU usage in extended quiescent state

Hitoshi Mitake (1):
asm-generic: architecture independent readq/writeq for 32bit environment

Huajun Li (1):
[SCSI] scsi_scan: Fix 'Poison overwritten' warning caused by
using freed 'shost'

Ian Kent (1):
autofs: work around unhappy compat problem on x86-64

Inki Dae (4):
drm/exynos: added possible_clones setup function. drm/exynos: fixed page flip issue. drm/exynos: removed exynos_drm_fbdev_recreate function. drm/exynos: added postclose to release resource.

James Morris (1):
maintainers: update my email address

Jan Kara (2):
vfs: Provide function to get superblock and wait for it to thaw
quota: Fix deadlock with suspend and quotas

Jan Schmidt (1):
Btrfs: avoid positive number with ERR_PTR

Janne Grunau (1):
[media] hdpvr: fix race conditon during start of streaming

Jarod Wilson (1):
[media] imon: don't wedge hardware after early callbacks

Javi Merino (2):
ARM: 7164/3: PL330: Fix the size of the dst_cache_ctrl field
ARM: 7326/2: PL330: fix null pointer dereference in pl330_chan_ctrl()

Jeff Liu (1):
Btrfs: return the internal error unchanged if
btrfs_get_extent_fiemap() call failed for SEEK_DATA/SEEK_HOLE inquiry

Jeff Mahoney (1):
btrfs: delalloc for page dirtied out-of-band in fixup worker

Jerome Glisse (1):
drm/radeon/kms: properly set accel working flag and bailout when false

Jesper Juhl (1):
XFS: xfs_trans_add_item() - don't assign in ASSERT() when
compare is intended

Joonyoung Shim (2):
drm/exynos: changed priority of mixer layers. drm/exynos: removed pageflip_event_list init code when closed.

Keith Mannthey (1):
btrfs: Sector Size check during Mount

Kenneth Graunke (1):
drm/i915: gen7: Disable the RHWO optimization as it can cause GPU hangs.

Kleber Sacilotto de Souza (1):
[SCSI] ipr: fix eeh recovery for 64-bit adapters

Kuninori Morimoto (6):
sh: clock-sh7724: fixup sh_fsi clock settings
ARM: mach-shmobile: mackerel: use renesas_usbhs instead of r8a66597_hcd
ARM: mach-shmobile: mackerel: add ak4642 amixer settings on comment
ARM: mach-shmobile: clock-sh73a0: add DSIxPHY clock support
sh: clkfwk: bugfix: use clk_reparent() for div6 clocks
ASoC: ak4642: fixup HeadPhone L/R dapm settings

Kyle McMartin (1):
MAINTAINERS: drop me from PA-RISC maintenance

Laurent Pinchart (1):
[media] omap3isp: Fix crash caused by subdevs now having a
pointer to devnodes

Linus Torvalds (6):
i387: fix up some fpu_counter confusion
i387: use 'restore_fpu_checking()' directly in task switching code
i387: support lazy restore of FPU state
i387: export 'fpu_owner_task' per-cpu variable
sys_poll: fix incorrect type for 'timeout' parameter Linux 3.3-rc5

Liu Bo (6):
Btrfs: fix trim 0 bytes after a device delete
Btrfs: skip states when they does not contain bits to clear
Btrfs: kick out redundant stuff in convert_extent_bit
Btrfs: fix a bug on overcommit stuff
Btrfs: be less strict on finding next node in clear_extent_bit
Btrfs: increase the global block reserve estimates

Lucas De Marchi (1):
kbuild: do not check for ancient modutils tools

Magnus Damm (5):
ARM: mach-shmobile: sh73a0 PINT IRQ base fix
ARM: mach-shmobile: sh73a0 IRQ sparse alloc fix
ARM: mach-shmobile: IRQ driven GPIO key support for Kota2
ARM: mach-shmobile: sh73a0 PSTR 32-bit access fix
ARM: mach-shmobile: r8a7779 PFC IPSR4 fix

Mark Brown (1):
ASoC: wm8962: Fix sidetone enumeration texts

Mark Hills (1):
ALSA: snd-usb-caiaq: Fix the return of XRUN

Martin Schwidefsky (3):
[S390] incorrect PageTables counter for kvm page tables [S390] 3215 deadlock with tty_wakeup
[S390] correct ktime to tod clock comparator conversion

Masanari Iida (2):
drm/exynos: Fix typo in exynos_mixer.c
sh: Fix typo in pci-sh7780.c

Miao Xie (1):
Btrfs: fix deadlock on page lock when doing auto-defragment

Michael Christie (1):
[SCSI] qla2xxx: Remove check for null fcport from host reset handler.

Michael Ellerman (1):
powerpc: Fix program check handling when lockdep is enabled

Michel Dänzer (1):
drm/radeon: Only create additional ring debugfs files on Cayman or newer.

Miklos Szeredi (1):
vfs: fix d_inode_lookup() dentry ref leak

Mitsuo Hayasaka (2):
xfs: change available ranges of softlimit and hardlimit in quota check
xfs: make inode quota check more general

Moger, Babu (1):
[SCSI] scsi_dh_rdac: Fix for unbalanced reference count

Myron Stowe (1):
ARM/PCI: Remove ARM's duplicate definition of 'pcibios_max_latency'

Nikolaus Schulz (1):
hwmon: (f75375s) Fix register write order when setting fans to full speed

Oleg Nesterov (2):

      epoll: introduce POLLFREE to flush ->signalfd_wqh before kfree()
      epoll: ep_unregister_pollwait() can use the freed pwq->whead

Olof Johansson (1):
ARM: 7327/1: need to include asm/system.h in asm/processor.h

Paul Gortmaker (1):
arm: fix compile failure in mach-shmobile/board-ag5evm.c

Paul Mundt (2):
sh: Defer to asm-generic/device.h.
video: pvr2fb: Fix up spurious section mismatch warnings.

Phil Edworthy (1):
sh: Fix sh2a build error for CONFIG_CACHE_WRITETHROUGH

Rabin Vincent (1):
ARM: 7325/1: fix v7 boot with lockdep enabled

Randy Dunlap (1):
[media] wl128x: fix build errors when GPIOLIB is not enabled

Russell King (1):
ARM: OMAP: fix voltage domain build errors with PM_OPP disabled

Rusty Russell (2):
powerpc: Remove references to cpu_*_map
arch/sh: remove references to cpu_*_map.

Santosh Shilimkar (1):
ARM: 7336/1: smp_twd: Don't register CPUFREQ notifiers if local
timers are not initialised

Sarah Sharp (5):
USB: Remove duplicate USB 3.0 hub feature #defines.
xhci: Fix oops caused by more USB2 ports than USB3 ports.
USB: Don't fail USB3 probe on missing legacy PCI IRQ.
USB: Fix handoff when BIOS disables host PCI device.
xhci: Fix encoding for HS bulk/control NAK rate.

Shimoda, Yoshihiro (4):
sh: fix the sh_mmcif_plat_data in board-sh7757lcr
sh: modify resource for SPI0 in setup-sh7757
sh: add platform_device for SPI1 in setup-sh7757
sh: modify a resource of sh_eth_giga1_resources in board-sh7757lcr

Shyam Sundar (1):
[SCSI] qla2xxx: Remove resetting memory during device
initialization for ISP82xx.

Stephen Warren (1):
Kbuild: Use dtc's -d (dependency) option

Steven Rostedt (1):
autofs4 - fix lockdep splat in autofs

Takashi Iwai (2):
ALSA: hda/realtek - Fix overflow of vol/sw check bitmap
ALSA: hda/realtek - Fix surround output regression on Acer Aspire 5935

Taylor Ralph (1):
[media] hdpvr: update picture controls to support firmware versions > 0.15

Tony Lindgren (1):
ARM: 7324/1: modpost: Fix section warnings for ARM for many compilers

Trond Myklebust (3):
NFSv4: Fix an Oops in the NFSv4 getacl code
NFSv4: Ensure we throw out bad delegation stateids on NFS4ERR_BAD_STATEID NFSv4.1: Fix a NFSv4.1 session initialisation regression

Tsutomu Itoh (3):
Btrfs: fix memory leak in load_free_space_cache()
Btrfs: fix return value check of extent_io_ops
Btrfs: check return value of lookup_extent_mapping() correctly

Weston Andros Adamson (1):
NFSv4: fix server_scope memory leak

Xi Wang (1):
ALSA: usb-audio: avoid integer overflow in create_fixed_stream_quirk()

[email protected] (1):
USB: option: cleanup zte 3g-dongle's pid in option.c

#1 lore raw

On 12-02-25 03:34 PM, Linus Torvalds wrote:
..

Quoted 7 lines

And while the FP state save problem is gone, if you have a 64-bit capable CPU but are still running a 32-bit distro, it really would be interesting to verify that a 64-bit kernel works for you without problems. Because it always *should* have, but clearly that wasn't always the case. It would be very interesting to hear from people who have perhaps tried and failed before and perhaps didn't even bother to report the failure? Maybe it's worth trying again?

..

The i8k driver is still b0rked for COMPAT use in linux-3.2.xx, and I don't think my patch got picked up by anyone for 3.3 yet:

(unmangled copy attached -- copy below is for viewing only)

-----snip-----

Fix the i8k (Dell Laptop) driver so that its misdefined ioctl's work with existing 32-bit userspace on a 64-bit kernel.

Without this patch, 32-bit binaries get EINVAL from most ioctls, due to incompatible ioctl numbering and lack of a compat_ioctl() function.

The problem is that the ioctls were originally defined using _IOR/_IOW macros, but the code is written assuming simpler _IO macros. Ugh. Too late to re-do all of that -- would break existing userspace.

So just fix things up so they work again.

This has been tested with existing pre-compiled binaries (i8kctl) on both a pure 32-bit system and a 64/32 system.

drivers/char/i8k.c +41 -17
@@ -29,6 +29,7 @@
 #include <linux/mutex.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
+#include <linux/compat.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
@@ -92,6 +93,23 @@
 static int i8k_open_fs(struct inode *inode, struct file *file);
 static long i8k_ioctl(struct file *, unsigned int, unsigned long);
 
+#ifdef CONFIG_COMPAT
+static long
+i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, int __user *argp);
+
+static long
+i8k_compat_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+	long ret;
+
+	mutex_lock(&i8k_mutex);
+	ret = i8k_ioctl_unlocked(fp, cmd, compat_ptr(arg));
+	mutex_unlock(&i8k_mutex);
+
+	return ret;
+}
+#endif
+
 static const struct file_operations i8k_fops = {
 	.owner		= THIS_MODULE,
 	.open		= i8k_open_fs,
@@ -99,6 +117,9 @@
 	.llseek		= seq_lseek,
 	.release	= single_release,
 	.unlocked_ioctl	= i8k_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl   = i8k_compat_ioctl,
+#endif
 };
 
 struct smm_regs {
@@ -315,54 +336,56 @@
 	return regs.eax == 1145651527 && regs.edx == 1145392204 ? 0 : -1;
 }
 
-static int
-i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
+static long
+i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, int __user *argp)
 {
 	int val = 0;
 	int speed;
 	unsigned char buff[16];
-	int __user *argp = (int __user *)arg;
 
 	if (!argp)
 		return -EINVAL;
 
-	switch (cmd) {
-	case I8K_BIOS_VERSION:
-		val = i8k_get_bios_version();
+	switch (_IOC_NR(cmd)) {
+	case (_IOC_NR(I8K_BIOS_VERSION)):
+	{
+		int i;
+		for (val = 0, i = 0; i < strlen(bios_version); i++)
+			val = (val << 8) | bios_version[i];
 		break;
-
-	case I8K_MACHINE_ID:
+	}
+	case (_IOC_NR(I8K_MACHINE_ID)):
 		memset(buff, 0, 16);
 		strlcpy(buff, i8k_get_dmi_data(DMI_PRODUCT_SERIAL), sizeof(buff));
 		break;
 
-	case I8K_FN_STATUS:
+	case (_IOC_NR(I8K_FN_STATUS)):
 		val = i8k_get_fn_status();
 		break;
 
-	case I8K_POWER_STATUS:
+	case (_IOC_NR(I8K_POWER_STATUS)):
 		val = i8k_get_power_status();
 		break;
 
-	case I8K_GET_TEMP:
+	case (_IOC_NR(I8K_GET_TEMP)):
 		val = i8k_get_temp(0);
 		break;
 
-	case I8K_GET_SPEED:
+	case (_IOC_NR(I8K_GET_SPEED)):
 		if (copy_from_user(&val, argp, sizeof(int)))
 			return -EFAULT;
 
 		val = i8k_get_fan_speed(val);
 		break;
 
-	case I8K_GET_FAN:
+	case (_IOC_NR(I8K_GET_FAN)):
 		if (copy_from_user(&val, argp, sizeof(int)))
 			return -EFAULT;
 
 		val = i8k_get_fan_status(val);
 		break;
 
-	case I8K_SET_FAN:
+	case (_IOC_NR(I8K_SET_FAN)):
 		if (restricted && !capable(CAP_SYS_ADMIN))
 			return -EPERM;
 
@@ -383,12 +406,12 @@
 		return val;
 
 	switch (cmd) {
-	case I8K_BIOS_VERSION:
+	case (_IOC_NR(I8K_BIOS_VERSION)):
 		if (copy_to_user(argp, &val, 4))
 			return -EFAULT;
 
 		break;
-	case I8K_MACHINE_ID:
+	case (_IOC_NR(I8K_MACHINE_ID)):
 		if (copy_to_user(argp, buff, 16))
 			return -EFAULT;
 
@@ -406,9 +429,10 @@
 static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 {
 	long ret;
+	int __user *argp = (int __user *)arg;
 
 	mutex_lock(&i8k_mutex);
-	ret = i8k_ioctl_unlocked(fp, cmd, arg);
+	ret = i8k_ioctl_unlocked(fp, cmd, argp);
 	mutex_unlock(&i8k_mutex);
 
 	return ret;

Fix the i8k (Dell Laptop) driver so that its misdefined ioctl's work with existing 32-bit userspace on a 64-bit kernel.

Without this patch, 32-bit binaries get EINVAL from most ioctls, due to incompatible ioctl numbering and lack of a compat_ioctl() function.

The problem is that the ioctls were originally defined using _IOR/_IOW macros, but the code is written assuming simpler _IO macros. Ugh. Too late to re-do all of that -- would break existing userspace.

So just fix things up so they work again.

This has been tested with existing pre-compiled binaries (i8kctl) on both a pure 32-bit system and a 64/32 system.

drivers/char/i8k.c +41 -17

134 lines not on this page · raw message

#2 lore raw
Quoted 3 lines · Mark Lord

The i8k driver is still b0rked for COMPAT use in linux-3.2.xx, and I don't think my patch got picked up by anyone for 3.3 yet:

I really don't like your patch.

It basically compares the ioctl argument by dropping almost all bits. That looks completely wrong. If I read the patch correctly, it will now match ioctl's that have *nothing* to do with the i8k ioctls, that just happen to have a few bits in common.

I also think that your explanation is wrong. The problem is not the use of _IOR() at all, the problem is that the data structure given *to* the _IOR() macro is complete grabage. For example:

  #define I8K_GET_SPEED           _IOWR('i', 0x85, size_t)

that "size_t" there is just crap. It's wrong. The ioctl doesn't write a size_t (which is different sizes on 32-bit and 64-bit), it actually writes an "int" (which happens to be the same size in both compat and non-compat).

So the 64-bit code - totally incorrectly - has been compiled with an ioctl number that implies a 64-bit argument.

EVERY SINGLE IOCTL that is defined for the i8k driver is crap. There are two that were "int", but they are marked as "broken" because the data structure *they* write isn't actually "int". But those two are the ones that just happen to work in both 32-bit and 64-bit mode, because at least the size of the (incorrect) data structure is the same.

The ones that have "size_t" aren't marked broken, but those are the *really* broken ones due to the wrong data structure choice that has a size that now is different in 32-bit and 64-bit mode.

Quite frankly, I think the right solution is to fix the kernel interface to the right type (int) that is the same. But because we don't want to change the user interface, let's make the kernel *accept* the 8-byte entity and just change it into a 4-byte size, and leave the user-space visible ioctl numbers the same broken crap - it's not like the other ioctl numbers had the right size *either*...

IOW, have something like the attached in the ioctl handler (and then we need to also add the compat handler, as in your patch).

Does this (with your compat_ioctl wrapper addition) also work for you?

Linus

 drivers/char/i8k.c  |    4 ++++
 include/linux/i8k.h |   22 ++++++++++++++++------
 2 files changed, 20 insertions(+), 6 deletions(-)
drivers/char/i8k.c +4 -0
index 40cc0cf2ded6..e4b1613543ed 100644
@@ -326,6 +326,10 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
 	if (!argp)
 		return -EINVAL;
 
+	/* We had some bad 64-bit confusion */
+	if ((arg >> _IOC_SIZESHIFT) & _IOC_SIZEMASK) == 8)
+		arg -= 4 << _IOC_SIZESHIFT;
+
 	switch (cmd) {
 	case I8K_BIOS_VERSION:
 		val = i8k_get_bios_version();
include/linux/i8k.h +16 -6
index 1c45ba505115..650f4015c89c 100644
@@ -20,14 +20,24 @@
 #define I8K_PROC		"/proc/i8k"
 #define I8K_PROC_FMT		"1.0"
 
+/*
+ * For hysterical raisins we expose the wrong size to user space
+ * in the ioctl numbering. The actual real data size is "int".
+ */
+#ifdef __KERNEL__
+#define borked_i8k_arg int
+#else
+#define borked_i8k_arg size_t
+#endif
+
 #define I8K_BIOS_VERSION	_IOR ('i', 0x80, int)	/* broken: meant 4 bytes */
 #define I8K_MACHINE_ID		_IOR ('i', 0x81, int)	/* broken: meant 16 bytes */
-#define I8K_POWER_STATUS	_IOR ('i', 0x82, size_t)
-#define I8K_FN_STATUS		_IOR ('i', 0x83, size_t)
-#define I8K_GET_TEMP		_IOR ('i', 0x84, size_t)
-#define I8K_GET_SPEED		_IOWR('i', 0x85, size_t)
-#define I8K_GET_FAN		_IOWR('i', 0x86, size_t)
-#define I8K_SET_FAN		_IOWR('i', 0x87, size_t)
+#define I8K_POWER_STATUS	_IOR ('i', 0x82, borked_i8k_arg)
+#define I8K_FN_STATUS		_IOR ('i', 0x83, borked_i8k_arg)
+#define I8K_GET_TEMP		_IOR ('i', 0x84, borked_i8k_arg)
+#define I8K_GET_SPEED		_IOWR('i', 0x85, borked_i8k_arg)
+#define I8K_GET_FAN		_IOWR('i', 0x86, borked_i8k_arg)
+#define I8K_SET_FAN		_IOWR('i', 0x87, borked_i8k_arg)
 
 #define I8K_FAN_LEFT		1
 #define I8K_FAN_RIGHT		0
#3 lore raw
Quoted 5 lines · Linus Torvalds

I also think that your explanation is wrong. The problem is not the use of _IOR() at all, the problem is that the data structure given *to* the _IOR() macro is complete grabage. For example:

  #define I8K_GET_SPEED           _IOWR('i', 0x85, size_t)

Originally the garbage looked like this:

#define I8K_BIOS_VERSION       _IOR ('i', 0x80, 4)
#define I8K_MACHINE_ID         _IOR ('i', 0x81, 16)
#define I8K_POWER_STATUS       _IOR ('i', 0x82, sizeof(int))
#define I8K_FN_STATUS          _IOR ('i', 0x83, sizeof(int))
#define I8K_GET_TEMP           _IOR ('i', 0x84, sizeof(int))
#define I8K_GET_SPEED          _IOWR('i', 0x85, sizeof(int))
#define I8K_GET_FAN            _IOWR('i', 0x86, sizeof(int))
#define I8K_SET_FAN            _IOWR('i', 0x87, sizeof(int)*2)

Later they were modified by "[PATCH] use size_t for the broken ioctl numbers" and "Fix more ioctl _IOR/_IOW misusage." (see tglx's history tree).

Andreas.

Signature · 3 lines
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
#4 lore raw
Quoted 12 lines · Andreas Schwab

Originally the garbage looked like this:

#define I8K_BIOS_VERSION       _IOR ('i', 0x80, 4) #define I8K_MACHINE_ID         _IOR ('i', 0x81, 16) #define I8K_POWER_STATUS       _IOR ('i', 0x82, sizeof(int)) #define I8K_FN_STATUS          _IOR ('i', 0x83, sizeof(int)) #define I8K_GET_TEMP           _IOR ('i', 0x84, sizeof(int)) #define I8K_GET_SPEED          _IOWR('i', 0x85, sizeof(int)) #define I8K_GET_FAN            _IOWR('i', 0x86, sizeof(int)) #define I8K_SET_FAN            _IOWR('i', 0x87, sizeof(int)*2)

Quoted 3 lines

Later they were modified by "[PATCH] use size_t for the broken ioctl numbers" and "Fix more ioctl _IOR/_IOW misusage." (see tglx's history tree).

Ahh, yes. That wasn't the only driver that did the "sizeof" by hand, and as a result got "size_t" when the (nested) *real* sizeof then happened.

That explains the history behind the garbage.

ioctl numbering have always been pretty painful. Oh, how I wish they had been ASCII strings rather than random binary blobs. But that's the way things are (and we certainly screwed up a few /proc files too, so it's not like using text fixes all potential for screwing up)

Linus

#5 lore raw
In reply to Linus Torvalds
Quoted 6 lines · Linus Torvalds
Quoted 3 lines · Mark Lord

The i8k driver is still b0rked for COMPAT use in linux-3.2.xx, and I don't think my patch got picked up by anyone for 3.3 yet:

I really don't like your patch.

..

Heh, I think we agree on that point. I hate it. :)

Quoted 11 lines

Quite frankly, I think the right solution is to fix the kernel interface to the right type (int) that is the same. But because we don't want to change the user interface, let's make the kernel *accept* the 8-byte entity and just change it into a 4-byte size, and leave the user-space visible ioctl numbers the same broken crap - it's not like the other ioctl numbers had the right size *either*...

IOW, have something like the attached in the ioctl handler (and then we need to also add the compat handler, as in your patch).

Does this (with your compat_ioctl wrapper addition) also work for you?

It's close (after adding a missing left-paren), but not 100% working yet. In particular, this command fails to get valid data: i8kctl bios

    ioctl(3, I8K_BIOS_VERSION, 0xbfc543c8)  = -1 EINVAL (Invalid argument)

That's the original issue I first noticed which prompted me to try and get things to roughly behave again.

I think all (or at least most) of the other ioctls appear to be working though.

#6 lore raw

PATCH 1/1 i8k: fix ioctl handing (was Re: Linux 3.3-rc5)

Quoted 2 lines · Mark Lord
Quoted 1 line · Linus Torvalds

On Mon, Feb 27, 2012 at 7:23 AM, Mark Lord <[email protected]> wrote:

..

Quoted 7 lines
Quoted 1 line

Does this (with your compat_ioctl wrapper addition) also work for you?

It's close (after adding a missing left-paren), but not 100% working yet. In particular, this command fails to get valid data: i8kctl bios

    ioctl(3, I8K_BIOS_VERSION, 0xbfc543c8)  = -1 EINVAL (Invalid argument)

Oh.. that's due to just plain broken code in i8k.c. My original patch had a fix for that too:

- switch (cmd) {
- case I8K_BIOS_VERSION:
- val = i8k_get_bios_version();

+       switch (_IOC_NR(cmd)) {
+       case (_IOC_NR(I8K_BIOS_VERSION)):
+       {
+               int i;
+               for (val = 0, i = 0; i < strlen(bios_version); i++)
+                       val = (val << 8) | bios_version[i];
                break;

So, with your patch + bits of mine, we may have it all working. Here's a single combined patch for further discussion.

I'm still not 100% happy with the arg/argp passing to i8ki_unlocked_ioctl() though. Shouldn't there be a compat_ptr() macro in there someplace?

----------------snip-----------------

Fix all kinds of badness in the i8k driver's ioctl handling. This enables it to continue to work with existing 32-bit binaries under both 32-bit and 64-bit kernels.

It also fixes the broken I8K_BIOS_VERSION call for all situations.

include/linux/i8k.h +16 -6
@@ -20,14 +20,24 @@
 #define I8K_PROC		"/proc/i8k"
 #define I8K_PROC_FMT		"1.0"
 
+/*
+ * For hysterical raisins we expose the wrong size to user space
+ * in the ioctl numbering. The actual real data size is "int".
+ */
+#ifdef __KERNEL__
+#define borked_i8k_arg int
+#else
+#define borked_i8k_arg size_t
+#endif
+
 #define I8K_BIOS_VERSION	_IOR ('i', 0x80, int)	/* broken: meant 4 bytes */
 #define I8K_MACHINE_ID		_IOR ('i', 0x81, int)	/* broken: meant 16 bytes */
-#define I8K_POWER_STATUS	_IOR ('i', 0x82, size_t)
-#define I8K_FN_STATUS		_IOR ('i', 0x83, size_t)
-#define I8K_GET_TEMP		_IOR ('i', 0x84, size_t)
-#define I8K_GET_SPEED		_IOWR('i', 0x85, size_t)
-#define I8K_GET_FAN		_IOWR('i', 0x86, size_t)
-#define I8K_SET_FAN		_IOWR('i', 0x87, size_t)
+#define I8K_POWER_STATUS	_IOR ('i', 0x82, borked_i8k_arg)
+#define I8K_FN_STATUS		_IOR ('i', 0x83, borked_i8k_arg)
+#define I8K_GET_TEMP		_IOR ('i', 0x84, borked_i8k_arg)
+#define I8K_GET_SPEED		_IOWR('i', 0x85, borked_i8k_arg)
+#define I8K_GET_FAN		_IOWR('i', 0x86, borked_i8k_arg)
+#define I8K_SET_FAN		_IOWR('i', 0x87, borked_i8k_arg)
 
 #define I8K_FAN_LEFT		1
 #define I8K_FAN_RIGHT		0
drivers/char/i8k.c +27 -3
@@ -92,6 +92,23 @@
 static int i8k_open_fs(struct inode *inode, struct file *file);
 static long i8k_ioctl(struct file *, unsigned int, unsigned long);
 
+#ifdef CONFIG_COMPAT
+static long
+i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg);
+
+static long
+i8k_compat_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+	long ret;
+
+	mutex_lock(&i8k_mutex);
+	ret = i8k_ioctl_unlocked(fp, cmd, arg);
+	mutex_unlock(&i8k_mutex);
+
+	return ret;
+}
+#endif
+
 static const struct file_operations i8k_fops = {
 	.owner		= THIS_MODULE,
 	.open		= i8k_open_fs,
@@ -99,6 +116,9 @@
 	.llseek		= seq_lseek,
 	.release	= single_release,
 	.unlocked_ioctl	= i8k_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl   = i8k_compat_ioctl,
+#endif
 };
 
 struct smm_regs {
@@ -318,17 +338,21 @@
 static int
 i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
 {
-	int val = 0;
-	int speed;
+	int i, speed, val = 0;
 	unsigned char buff[16];
 	int __user *argp = (int __user *)arg;
 
 	if (!argp)
 		return -EINVAL;
 
+	/* We had some bad 64-bit confusion */
+	if (((arg >> _IOC_SIZESHIFT) & _IOC_SIZEMASK) == 8)
+		arg -= 4 << _IOC_SIZESHIFT;
+
 	switch (cmd) {
 	case I8K_BIOS_VERSION:
-		val = i8k_get_bios_version();
+		for (val = 0, i = 0; i < strlen(bios_version); i++)
+			val = (val << 8) | bios_version[i];
 		break;
 
 	case I8K_MACHINE_ID:

Fix all kinds of badness in the i8k driver's ioctl handling. This enables it to continue to work with existing 32-bit binaries under both 32-bit and 64-bit kernels.

It also fixes the broken I8K_BIOS_VERSION call for all situations.

include/linux/i8k.h +16 -6
@@ -20,14 +20,24 @@
 #define I8K_PROC		"/proc/i8k"
 #define I8K_PROC_FMT		"1.0"
 
+/*
+ * For hysterical raisins we expose the wrong size to user space
+ * in the ioctl numbering. The actual real data size is "int".
+ */
+#ifdef __KERNEL__
+#define borked_i8k_arg int
+#else
+#define borked_i8k_arg size_t
+#endif
+
 #define I8K_BIOS_VERSION	_IOR ('i', 0x80, int)	/* broken: meant 4 bytes */
 #define I8K_MACHINE_ID		_IOR ('i', 0x81, int)	/* broken: meant 16 bytes */
-#define I8K_POWER_STATUS	_IOR ('i', 0x82, size_t)
-#define I8K_FN_STATUS		_IOR ('i', 0x83, size_t)
-#define I8K_GET_TEMP		_IOR ('i', 0x84, size_t)
-#define I8K_GET_SPEED		_IOWR('i', 0x85, size_t)
-#define I8K_GET_FAN		_IOWR('i', 0x86, size_t)
-#define I8K_SET_FAN		_IOWR('i', 0x87, size_t)
+#define I8K_POWER_STATUS	_IOR ('i', 0x82, borked_i8k_arg)
+#define I8K_FN_STATUS		_IOR ('i', 0x83, borked_i8k_arg)
+#define I8K_GET_TEMP		_IOR ('i', 0x84, borked_i8k_arg)
+#define I8K_GET_SPEED		_IOWR('i', 0x85, borked_i8k_arg)
+#define I8K_GET_FAN		_IOWR('i', 0x86, borked_i8k_arg)
+#define I8K_SET_FAN		_IOWR('i', 0x87, borked_i8k_arg)
 
 #define I8K_FAN_LEFT		1
 #define I8K_FAN_RIGHT		0
drivers/char/i8k.c +27 -3
@@ -92,6 +92,23 @@
 static int i8k_open_fs(struct inode *inode, struct file *file);
 static long i8k_ioctl(struct file *, unsigned int, unsigned long);
 
+#ifdef CONFIG_COMPAT
+static long
+i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg);
+
+static long
+i8k_compat_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+	long ret;
+
+	mutex_lock(&i8k_mutex);
+	ret = i8k_ioctl_unlocked(fp, cmd, arg);
+	mutex_unlock(&i8k_mutex);
+
+	return ret;
+}
+#endif
+
 static const struct file_operations i8k_fops = {
 	.owner		= THIS_MODULE,
 	.open		= i8k_open_fs,
@@ -99,6 +116,9 @@
 	.llseek		= seq_lseek,
 	.release	= single_release,
 	.unlocked_ioctl	= i8k_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl   = i8k_compat_ioctl,
+#endif
 };
 
 struct smm_regs {
@@ -318,17 +338,21 @@
 static int
 i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
 {
-	int val = 0;
-	int speed;
+	int i, speed, val = 0;
 	unsigned char buff[16];
 	int __user *argp = (int __user *)arg;
 
 	if (!argp)
 		return -EINVAL;
 
+	/* We had some bad 64-bit confusion */
+	if (((arg >> _IOC_SIZESHIFT) & _IOC_SIZEMASK) == 8)
+		arg -= 4 << _IOC_SIZESHIFT;
+
 	switch (cmd) {
 	case I8K_BIOS_VERSION:
-		val = i8k_get_bios_version();
+		for (val = 0, i = 0; i < strlen(bios_version); i++)
+			val = (val << 8) | bios_version[i];
 		break;
 
 	case I8K_MACHINE_ID:
#7 lore raw

Re: [PATCH] i8k: fix ioctl handing (was Re: Linux 3.3-rc5)

Quoted 3 lines · Mark Lord

I'm still not 100% happy with the arg/argp passing to i8ki_unlocked_ioctl() though. Shouldn't there be a compat_ptr() macro in there someplace?

Yes, I think you should have "compat_ptr(arg)" there.

Patch looks good. And perhaps i8k_get_bios_version() should be renamed to "i8k_get_smm_bios_version()" to not cause this crazy confusion again.

Linus

#8 lore raw
In reply to Mark Lord
Quoted 5 lines · Mark Lord

It's close (after adding a missing left-paren), but not 100% working yet. In particular, this command fails to get valid data:  i8kctl bios

   ioctl(3, I8K_BIOS_VERSION, 0xbfc543c8)  = -1 EINVAL (Invalid argument)

Hmm. That makes no sense.

I8K_BIOS_VERSION is 0x80046980 - and it already matches on both x86-64 and x86-32. I just checked by compiling to asm. And the size is '4', so it wouldn't trigger any changes by the sizemask thing either.

Ugh. But my patch was crap. It fixed up "arg", but it *should* have fixed up "cmd".

Stupid.

So try *this* one instead. I also fixed up the missing parenthesis, but didn't add the actual .compat function etc.

Does *this* work? And if not, can you do an strace, but use the "-e raw=ioctl" option to make it show the ioctl numbers as raw hex to verify..

Linus

Linus

 drivers/char/i8k.c  |    4 ++++
 include/linux/i8k.h |   22 ++++++++++++++++------
 2 files changed, 20 insertions(+), 6 deletions(-)
drivers/char/i8k.c +4 -0
index 40cc0cf2ded6..2baa921afd84 100644
@@ -326,6 +326,10 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
 	if (!argp)
 		return -EINVAL;
 
+	/* We had some bad 64-bit confusion */
+	if (((cmd >> _IOC_SIZESHIFT) & _IOC_SIZEMASK) == 8)
+		cmd -= 4 << _IOC_SIZESHIFT;
+
 	switch (cmd) {
 	case I8K_BIOS_VERSION:
 		val = i8k_get_bios_version();
include/linux/i8k.h +16 -6
index 1c45ba505115..650f4015c89c 100644
@@ -20,14 +20,24 @@
 #define I8K_PROC		"/proc/i8k"
 #define I8K_PROC_FMT		"1.0"
 
+/*
+ * For hysterical raisins we expose the wrong size to user space
+ * in the ioctl numbering. The actual real data size is "int".
+ */
+#ifdef __KERNEL__
+#define borked_i8k_arg int
+#else
+#define borked_i8k_arg size_t
+#endif
+
 #define I8K_BIOS_VERSION	_IOR ('i', 0x80, int)	/* broken: meant 4 bytes */
 #define I8K_MACHINE_ID		_IOR ('i', 0x81, int)	/* broken: meant 16 bytes */
-#define I8K_POWER_STATUS	_IOR ('i', 0x82, size_t)
-#define I8K_FN_STATUS		_IOR ('i', 0x83, size_t)
-#define I8K_GET_TEMP		_IOR ('i', 0x84, size_t)
-#define I8K_GET_SPEED		_IOWR('i', 0x85, size_t)
-#define I8K_GET_FAN		_IOWR('i', 0x86, size_t)
-#define I8K_SET_FAN		_IOWR('i', 0x87, size_t)
+#define I8K_POWER_STATUS	_IOR ('i', 0x82, borked_i8k_arg)
+#define I8K_FN_STATUS		_IOR ('i', 0x83, borked_i8k_arg)
+#define I8K_GET_TEMP		_IOR ('i', 0x84, borked_i8k_arg)
+#define I8K_GET_SPEED		_IOWR('i', 0x85, borked_i8k_arg)
+#define I8K_GET_FAN		_IOWR('i', 0x86, borked_i8k_arg)
+#define I8K_SET_FAN		_IOWR('i', 0x87, borked_i8k_arg)
 
 #define I8K_FAN_LEFT		1
 #define I8K_FAN_RIGHT		0
#9 lore raw
Quoted 8 lines · Linus Torvalds
Quoted 5 lines · Mark Lord

It's close (after adding a missing left-paren), but not 100% working yet. In particular, this command fails to get valid data: i8kctl bios

   ioctl(3, I8K_BIOS_VERSION, 0xbfc543c8)  = -1 EINVAL (Invalid argument)

Hmm. That makes no sense.

No, that was my misinterpretation.
The BIOS_VERSION call is broken for different reasons, and my original all-inclusive patch has a fix for it too. :)

Quoted 3 lines

Does *this* work? And if not, can you do an strace, but use the "-e raw=ioctl" option to make it show the ioctl numbers as raw hex to verify..

Here's a go, 64-bit kernel (3.2.8), 32-bit userspace:

Dell laptop SMM driver v1.14 21/02/2005 Massimo Dal Zotto ([email protected]) ioctl32(i8kctl:1474): Unknown cmd fd(3) cmd(80046980){t:'i';sz:4} arg(ff8b1fd8) on /proc/i8k
ioctl32(i8kctl:1475): Unknown cmd fd(3) cmd(c0046986){t:'i';sz:4} arg(ff90ed5c) on /proc/i8k
ioctl32(i8kctl:1475): Unknown cmd fd(3) cmd(c0046986){t:'i';sz:4} arg(ff90ed5c) on /proc/i8k

I'll reboot/redo that again with strace shortly.

#10 lore raw

On 12-03-01 07:17 PM, Mark Lord wrote:
..

Quoted 11 lines

Here's a go, 64-bit kernel (3.2.8), 32-bit userspace:

Dell laptop SMM driver v1.14 21/02/2005 Massimo Dal Zotto ([email protected]) ioctl32(i8kctl:1474): Unknown cmd fd(3) cmd(80046980){t:'i';sz:4} arg(ff8b1fd8) on /proc/i8k
ioctl32(i8kctl:1475): Unknown cmd fd(3) cmd(c0046986){t:'i';sz:4} arg(ff90ed5c) on /proc/i8k
ioctl32(i8kctl:1475): Unknown cmd fd(3) cmd(c0046986){t:'i';sz:4} arg(ff90ed5c) on /proc/i8k

I'll reboot/redo that again with strace shortly.

It all seems to work in 32-bit/32-bit mode, by the way.

Here's the promised strace from 64/32 mode:

$ strace -e raw=ioctl i8kctl fan

execve("/usr/bin/i8kctl", ["i8kctl", "fan"], [/* 21 vars */]) = 0
brk(0) = 0x8e6a000
access("/etc/ld.so.nohwcap", F_OK) = -1 ENOENT (No such file or directory)
mmap2(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xf77c9000
access("/etc/ld.so.preload", R_OK) = -1 ENOENT (No such file or directory)
open("/etc/ld.so.cache", O_RDONLY) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=123776, ...}) = 0 mmap2(NULL, 123776, PROT_READ, MAP_PRIVATE, 3, 0) = 0xf77aa000
close(3) = 0
access("/etc/ld.so.nohwcap", F_OK) = -1 ENOENT (No such file or directory)
open("/lib/tls/i686/cmov/libc.so.6", O_RDONLY) = 3
read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0000m\1\0004\0\0\0"...,
512) = 512
fstat64(3, {st_mode=S_IFREG|0755, st_size=1405508, ...}) = 0 mmap2(NULL, 1415592, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0xf7650000
mprotect(0xf77a3000, 4096, PROT_NONE) = 0
mmap2(0xf77a4000, 12288, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x153) = 0xf77a4000 mmap2(0xf77a7000, 10664, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0xf77a7000
close(3) = 0
mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xf764f000
set_thread_area({entry_number:-1 -> 12, base_addr:0xf764f8d0, limit:1048575, seg_32bit:1, contents:0, read_exec_only:0, limit_in_pages:1, seg_not_present:0, useable:1}) = 0

mprotect(0xf77a4000, 8192, PROT_READ)   = 0
mprotect(0x804a000, 4096, PROT_READ)    = 0
mprotect(0xf77e7000, 4096, PROT_READ)   = 0
munmap(0xf77aa000, 123776)              = 0
open("/proc/i8k", O_RDONLY)             = 3
ioctl(0x3, 0xc0046986, 0xffc06dcc)      = -1 (errno 22)
ioctl(0x3, 0xc0046986, 0xffc06dcc)      = -1 (errno 22)

fstat64(1, {st_mode=S_IFREG|0644, st_size=1848, ...}) = 0 mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xf77c8000
write(1, "-1 -1\n", 6-1 -1
) = 6
exit_group(0) = ?

and from dmesg:

ioctl32(i8kctl:1434): Unknown cmd fd(3) cmd(c0046986){t:'i';sz:4} arg(ffc06dcc) on /proc/i8k
ioctl32(i8kctl:1434): Unknown cmd fd(3) cmd(c0046986){t:'i';sz:4} arg(ffc06dcc) on /proc/i8k

#11 lore raw
Quoted 2 lines · Mark Lord

Here's the promised strace from 64/32 mode:

This is odd.

Quoted 1 line

ioctl(0x3, 0xc0046986, 0xffc06dcc)      = -1 (errno 22)

When I do a "make drivers/char/i8k.s" (with my patch), I can see that constant in the resulting code. It's right there. It doesn't return EINVAL.

Can you send me the patch you are using with the compat_ioctl thing added?

Linus

#12 lore raw
Quoted 6 lines · Linus Torvalds
Quoted 2 lines · Mark Lord

Here's the promised strace from 64/32 mode:

This is odd.

Quoted 7 lines
Quoted 1 line

ioctl(0x3, 0xc0046986, 0xffc06dcc) = -1 (errno 22)

When I do a "make drivers/char/i8k.s" (with my patch), I can see that constant in the resulting code. It's right there. It doesn't return EINVAL.

Can you send me the patch you are using with the compat_ioctl thing added?

Yeah.. here's the version I was testing with just now, which generated the errno above:

include/linux/i8k.h +16 -6
@@ -20,14 +20,24 @@
 #define I8K_PROC		"/proc/i8k"
 #define I8K_PROC_FMT		"1.0"
 
+/*
+ * For hysterical raisins we expose the wrong size to user space
+ * in the ioctl numbering. The actual real data size is "int".
+ */
+#ifdef __KERNEL__
+#define borked_i8k_arg int
+#else
+#define borked_i8k_arg size_t
+#endif
+
 #define I8K_BIOS_VERSION	_IOR ('i', 0x80, int)	/* broken: meant 4 bytes */
 #define I8K_MACHINE_ID		_IOR ('i', 0x81, int)	/* broken: meant 16 bytes */
-#define I8K_POWER_STATUS	_IOR ('i', 0x82, size_t)
-#define I8K_FN_STATUS		_IOR ('i', 0x83, size_t)
-#define I8K_GET_TEMP		_IOR ('i', 0x84, size_t)
-#define I8K_GET_SPEED		_IOWR('i', 0x85, size_t)
-#define I8K_GET_FAN		_IOWR('i', 0x86, size_t)
-#define I8K_SET_FAN		_IOWR('i', 0x87, size_t)
+#define I8K_POWER_STATUS	_IOR ('i', 0x82, borked_i8k_arg)
+#define I8K_FN_STATUS		_IOR ('i', 0x83, borked_i8k_arg)
+#define I8K_GET_TEMP		_IOR ('i', 0x84, borked_i8k_arg)
+#define I8K_GET_SPEED		_IOWR('i', 0x85, borked_i8k_arg)
+#define I8K_GET_FAN		_IOWR('i', 0x86, borked_i8k_arg)
+#define I8K_SET_FAN		_IOWR('i', 0x87, borked_i8k_arg)
 
 #define I8K_FAN_LEFT		1
 #define I8K_FAN_RIGHT		0
drivers/char/i8k.c +27 -3
@@ -92,6 +92,23 @@
 static int i8k_open_fs(struct inode *inode, struct file *file);
 static long i8k_ioctl(struct file *, unsigned int, unsigned long);
 
+#ifdef CONFIG_COMPAT
+static long
+i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg);
+
+static long
+i8k_compat_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+	long ret;
+
+	mutex_lock(&i8k_mutex);
+	ret = i8k_ioctl_unlocked(fp, cmd, arg);
+	mutex_unlock(&i8k_mutex);
+
+	return ret;
+}
+#endif
+
 static const struct file_operations i8k_fops = {
 	.owner		= THIS_MODULE,
 	.open		= i8k_open_fs,
@@ -99,6 +116,9 @@
 	.llseek		= seq_lseek,
 	.release	= single_release,
 	.unlocked_ioctl	= i8k_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl   = i8k_compat_ioctl,
+#endif
 };
 
 struct smm_regs {
@@ -318,17 +338,21 @@
 static int
 i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
 {
-	int val = 0;
-	int speed;
+	int i, speed, val = 0;
 	unsigned char buff[16];
 	int __user *argp = (int __user *)arg;
 
 	if (!argp)
 		return -EINVAL;
 
+	/* We had some bad 64-bit confusion */
+	if (((cmd >> _IOC_SIZESHIFT) & _IOC_SIZEMASK) == 8)
+		cmd -= 4 << _IOC_SIZESHIFT;
+
 	switch (cmd) {
 	case I8K_BIOS_VERSION:
-		val = i8k_get_bios_version();
+		for (val = 0, i = 0; i < strlen(bios_version); i++)
+			val = (val << 8) | bios_version[i];
 		break;
 
 	case I8K_MACHINE_ID:

Fix all kinds of badness in the i8k driver's ioctl handling. This enables it to continue to work with existing 32-bit binaries under both 32-bit and 64-bit kernels.

It also fixes the broken I8K_BIOS_VERSION call for all situations.

include/linux/i8k.h +16 -6
@@ -20,14 +20,24 @@
 #define I8K_PROC		"/proc/i8k"
 #define I8K_PROC_FMT		"1.0"
 
+/*
+ * For hysterical raisins we expose the wrong size to user space
+ * in the ioctl numbering. The actual real data size is "int".
+ */
+#ifdef __KERNEL__
+#define borked_i8k_arg int
+#else
+#define borked_i8k_arg size_t
+#endif
+
 #define I8K_BIOS_VERSION	_IOR ('i', 0x80, int)	/* broken: meant 4 bytes */
 #define I8K_MACHINE_ID		_IOR ('i', 0x81, int)	/* broken: meant 16 bytes */
-#define I8K_POWER_STATUS	_IOR ('i', 0x82, size_t)
-#define I8K_FN_STATUS		_IOR ('i', 0x83, size_t)
-#define I8K_GET_TEMP		_IOR ('i', 0x84, size_t)
-#define I8K_GET_SPEED		_IOWR('i', 0x85, size_t)
-#define I8K_GET_FAN		_IOWR('i', 0x86, size_t)
-#define I8K_SET_FAN		_IOWR('i', 0x87, size_t)
+#define I8K_POWER_STATUS	_IOR ('i', 0x82, borked_i8k_arg)
+#define I8K_FN_STATUS		_IOR ('i', 0x83, borked_i8k_arg)
+#define I8K_GET_TEMP		_IOR ('i', 0x84, borked_i8k_arg)
+#define I8K_GET_SPEED		_IOWR('i', 0x85, borked_i8k_arg)
+#define I8K_GET_FAN		_IOWR('i', 0x86, borked_i8k_arg)
+#define I8K_SET_FAN		_IOWR('i', 0x87, borked_i8k_arg)
 
 #define I8K_FAN_LEFT		1
 #define I8K_FAN_RIGHT		0
drivers/char/i8k.c +27 -3
@@ -92,6 +92,23 @@
 static int i8k_open_fs(struct inode *inode, struct file *file);
 static long i8k_ioctl(struct file *, unsigned int, unsigned long);
 
+#ifdef CONFIG_COMPAT
+static long
+i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg);
+
+static long
+i8k_compat_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+	long ret;
+
+	mutex_lock(&i8k_mutex);
+	ret = i8k_ioctl_unlocked(fp, cmd, arg);
+	mutex_unlock(&i8k_mutex);
+
+	return ret;
+}
+#endif
+
 static const struct file_operations i8k_fops = {
 	.owner		= THIS_MODULE,
 	.open		= i8k_open_fs,
@@ -99,6 +116,9 @@
 	.llseek		= seq_lseek,
 	.release	= single_release,
 	.unlocked_ioctl	= i8k_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl   = i8k_compat_ioctl,
+#endif
 };
 
 struct smm_regs {
@@ -318,17 +338,21 @@
 static int
 i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
 {
-	int val = 0;
-	int speed;
+	int i, speed, val = 0;
 	unsigned char buff[16];
 	int __user *argp = (int __user *)arg;
 
 	if (!argp)
 		return -EINVAL;
 
+	/* We had some bad 64-bit confusion */
+	if (((cmd >> _IOC_SIZESHIFT) & _IOC_SIZEMASK) == 8)
+		cmd -= 4 << _IOC_SIZESHIFT;
+
 	switch (cmd) {
 	case I8K_BIOS_VERSION:
-		val = i8k_get_bios_version();
+		for (val = 0, i = 0; i < strlen(bios_version); i++)
+			val = (val << 8) | bios_version[i];
 		break;
 
 	case I8K_MACHINE_ID:
#13 lore raw
In reply to Linus Torvalds
Quoted 23 lines · Linus Torvalds

Hey, no delays this week.

And nothing really odd going on either. Maybe things are finally calming down.

Sure, I'd have liked it to be even calmer, and there's is movement in various areas: btrfs updates along with scsi and media driver updates. And various noise elsewhere. But on the whole it's been pretty boring, which is just how I like it.

A couple of the fixes were directly linked to my -rc4 announcement, where I told people they could use a 64-bit kernel to avoid the FP state save/restore problem we used to have. That brought up some fairly corner-case compatibility issues that could make that impractical. So hopefully it's *true* this time around.

And while the FP state save problem is gone, if you have a 64-bit capable CPU but are still running a 32-bit distro, it really would be interesting to verify that a 64-bit kernel works for you without problems. Because it always *should* have, but clearly that wasn't always the case. It would be very interesting to hear from people who have perhaps tried and failed before and perhaps didn't even bother to report the failure? Maybe it's worth trying again?

Quoted 1 line

Linus

Yes, I just tried.

IIRC, I used to run 64 bit upstream kernels with 32 bit userspace under Fedora 14 (because I can) this stopped booting when I switched to Fedora 15, I think the upstream kernel was 2.6.39 at that time, so I stopped trying.

With 3.3.0-rc5, a 64 bit kernel works fine for me, again. Yay!

As a side note, it's a pity that gcc in 32 bit Fedora does not support -m 64 to support building 64 bit kernels.

Signature · 1 line
MST