Re: [PATCH] Fix/add raw1394 CONFIG_COMPAT code

[Dan Dennedy <dan@dennedy.org>]

On Sunday 06 May 2007 19:14, Petr Vandrovec wrote:
> Hello Dan,
> 
> This patch makes raw1394 in current Linux git tree (2.6.21-1570) usable to 32bit
> applications running on 64bit kernel (tested on i386 app using x86_64 kernel).  I 
> had to make following changes:
> 
> * read() always failed with -EFAULT.  This was happening due to
>   raw1394_compat_read copying data to wrong location - access_ok always
>   failed as 'r' is kernel address, not user.  Whole function just tried to
>   copy data from 'r' to 'r', which is not good.
> 
> * write(fd, buf, 52) from 32bit app was returning 56.  Most of callers did not
>   care, but some (arm registration) did, and anyway it looks bad if request for
>   writing 52 bytes returns 56.  And returning sizeof anything in 'int' is not
>   good as well.  So all functions now return '0' instead of
>   sizeof(struct raw1394_request) on success, and write() itself provides correct
>   return value (it just returns value it was asked to write on success as raw1394
>   does not do any partial writes at all).
> 
> * Related to this was problem that write() could have returned 0 when kernel
>   state would become corrupted and moved to different state than
>   opened/initialized/connected.  Now it returns -EBADFD which seemed appropriate.
> 
> * And add compat_ioctl.  Although all structures are more or less same,
>   raw1394_iso_packets got pointer inside, and raw1394_cycle_timer got unwanted
>   padding in the middle.  I did not add any translation for ioctls passing array
>   of integers around as integers seem to have same size (32 bits) on all 
>   architectures supported by Linux.
> 
> With this in place I was able to run my test app and grab some mpegs, so I believe
> that I got everything necessary done.  If you believe I have missed some ioctl, yell
> at me.
> 						Thanks,
> 							Petr Vandrovec
> 
> Signed-off-by: Petr Vandrovec <petr@vandrovec.name>

Reviewed and been testing out fine here on i386.
Acked-by: Dan Dennedy <dan@dennedy.org>

> diff -uprdN linux/drivers/ieee1394/raw1394.c linux/drivers/ieee1394/raw1394.c
> --- linux/drivers/ieee1394/raw1394.c	2007-05-06 17:45:47.000000000 -0700
> +++ linux/drivers/ieee1394/raw1394.c	2007-05-06 18:08:05.000000000 -0700
> @@ -460,7 +460,7 @@ static const char __user *raw1394_compat
>  static int
>  raw1394_compat_read(const char __user *buf, struct raw1394_request *r)
>  {
> -	struct compat_raw1394_req __user *cr = (typeof(cr)) r;
> +	struct compat_raw1394_req __user *cr = (typeof(cr)) buf;
>  	if (!access_ok(VERIFY_WRITE, cr, sizeof(struct compat_raw1394_req)) ||
>  	    P(type) ||
>  	    P(error) ||
> @@ -588,7 +588,7 @@ static int state_opened(struct file_info
>  
>  	req->req.length = 0;
>  	queue_complete_req(req);
> -	return sizeof(struct raw1394_request);
> +	return 0;
>  }
>  
>  static int state_initialized(struct file_info *fi, struct pending_request *req)
> @@ -602,7 +602,7 @@ static int state_initialized(struct file
>  		req->req.generation = atomic_read(&internal_generation);
>  		req->req.length = 0;
>  		queue_complete_req(req);
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  	}
>  
>  	switch (req->req.type) {
> @@ -674,7 +674,7 @@ out_set_card:
>  	}
>  
>  	queue_complete_req(req);
> -	return sizeof(struct raw1394_request);
> +	return 0;
>  }
>  
>  static void handle_iso_listen(struct file_info *fi, struct pending_request *req)
> @@ -866,7 +866,7 @@ static int handle_async_request(struct f
>  	if (req->req.error) {
>  		req->req.length = 0;
>  		queue_complete_req(req);
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  	}
>  
>  	hpsb_set_packet_complete_task(packet,
> @@ -884,7 +884,7 @@ static int handle_async_request(struct f
>  		hpsb_free_tlabel(packet);
>  		queue_complete_req(req);
>  	}
> -	return sizeof(struct raw1394_request);
> +	return 0;
>  }
>  
>  static int handle_iso_send(struct file_info *fi, struct pending_request *req,
> @@ -908,7 +908,7 @@ static int handle_iso_send(struct file_i
>  		req->req.error = RAW1394_ERROR_MEMFAULT;
>  		req->req.length = 0;
>  		queue_complete_req(req);
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  	}
>  
>  	req->req.length = 0;
> @@ -928,7 +928,7 @@ static int handle_iso_send(struct file_i
>  		queue_complete_req(req);
>  	}
>  
> -	return sizeof(struct raw1394_request);
> +	return 0;
>  }
>  
>  static int handle_async_send(struct file_info *fi, struct pending_request *req)
> @@ -943,7 +943,7 @@ static int handle_async_send(struct file
>  		req->req.error = RAW1394_ERROR_INVALID_ARG;
>  		req->req.length = 0;
>  		queue_complete_req(req);
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  	}
>  
>  	packet = hpsb_alloc_packet(req->req.length - header_length);
> @@ -956,7 +956,7 @@ static int handle_async_send(struct file
>  		req->req.error = RAW1394_ERROR_MEMFAULT;
>  		req->req.length = 0;
>  		queue_complete_req(req);
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  	}
>  
>  	if (copy_from_user
> @@ -965,7 +965,7 @@ static int handle_async_send(struct file
>  		req->req.error = RAW1394_ERROR_MEMFAULT;
>  		req->req.length = 0;
>  		queue_complete_req(req);
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  	}
>  
>  	packet->type = hpsb_async;
> @@ -993,7 +993,7 @@ static int handle_async_send(struct file
>  		queue_complete_req(req);
>  	}
>  
> -	return sizeof(struct raw1394_request);
> +	return 0;
>  }
>  
>  static int arm_read(struct hpsb_host *host, int nodeid, quadlet_t * buffer,
> @@ -1868,7 +1868,7 @@ static int arm_register(struct file_info
>  		spin_lock_irqsave(&host_info_lock, flags);
>  		list_add_tail(&addr->addr_list, &fi->addr_list);
>  		spin_unlock_irqrestore(&host_info_lock, flags);
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  	}
>  	retval =
>  	    hpsb_register_addrspace(&raw1394_highlevel, fi->host, &arm_ops,
> @@ -1886,7 +1886,7 @@ static int arm_register(struct file_info
>  		return (-EALREADY);
>  	}
>  	free_pending_request(req);	/* immediate success or fail */
> -	return sizeof(struct raw1394_request);
> +	return 0;
>  }
>  
>  static int arm_unregister(struct file_info *fi, struct pending_request *req)
> @@ -1954,7 +1954,7 @@ static int arm_unregister(struct file_in
>  		vfree(addr->addr_space_buffer);
>  		kfree(addr);
>  		free_pending_request(req);	/* immediate success or fail */
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  	}
>  	retval =
>  	    hpsb_unregister_addrspace(&raw1394_highlevel, fi->host,
> @@ -1970,7 +1970,7 @@ static int arm_unregister(struct file_in
>  	vfree(addr->addr_space_buffer);
>  	kfree(addr);
>  	free_pending_request(req);	/* immediate success or fail */
> -	return sizeof(struct raw1394_request);
> +	return 0;
>  }
>  
>  /* Copy data from ARM buffer(s) to user buffer. */
> @@ -2012,7 +2012,7 @@ static int arm_get_buf(struct file_info 
>  				 * queue no response, and therefore nobody
>  				 * will free it. */
>  				free_pending_request(req);
> -				return sizeof(struct raw1394_request);
> +				return 0;
>  			} else {
>  				DBGMSG("arm_get_buf request exceeded mapping");
>  				spin_unlock_irqrestore(&host_info_lock, flags);
> @@ -2064,7 +2064,7 @@ static int arm_set_buf(struct file_info 
>  				 * queue no response, and therefore nobody
>  				 * will free it. */
>  				free_pending_request(req);
> -				return sizeof(struct raw1394_request);
> +				return 0;
>  			} else {
>  				DBGMSG("arm_set_buf request exceeded mapping");
>  				spin_unlock_irqrestore(&host_info_lock, flags);
> @@ -2085,7 +2085,7 @@ static int reset_notification(struct fil
>  	    (req->req.misc == RAW1394_NOTIFY_ON)) {
>  		fi->notification = (u8) req->req.misc;
>  		free_pending_request(req);	/* we have to free the request, because we queue no response, and therefore nobody will free it */
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  	}
>  	/* error EINVAL (22) invalid argument */
>  	return (-EINVAL);
> @@ -2118,12 +2118,12 @@ static int write_phypacket(struct file_i
>  		req->req.length = 0;
>  		queue_complete_req(req);
>  	}
> -	return sizeof(struct raw1394_request);
> +	return 0;
>  }
>  
>  static int get_config_rom(struct file_info *fi, struct pending_request *req)
>  {
> -	int ret = sizeof(struct raw1394_request);
> +	int ret = 0;
>  	quadlet_t *data = kmalloc(req->req.length, GFP_KERNEL);
>  	int status;
>  
> @@ -2153,7 +2153,7 @@ static int get_config_rom(struct file_in
>  
>  static int update_config_rom(struct file_info *fi, struct pending_request *req)
>  {
> -	int ret = sizeof(struct raw1394_request);
> +	int ret = 0;
>  	quadlet_t *data = kmalloc(req->req.length, GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
> @@ -2220,7 +2220,7 @@ static int modify_config_rom(struct file
>  
>  			hpsb_update_config_rom_image(fi->host);
>  			free_pending_request(req);
> -			return sizeof(struct raw1394_request);
> +			return 0;
>  		}
>  	}
>  
> @@ -2285,7 +2285,7 @@ static int modify_config_rom(struct file
>  		/* we have to free the request, because we queue no response,
>  		 * and therefore nobody will free it */
>  		free_pending_request(req);
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  	} else {
>  		for (dentry =
>  		     fi->csr1212_dirs[dr]->value.directory.dentries_head;
> @@ -2310,7 +2310,7 @@ static int state_connected(struct file_i
>  
>  	case RAW1394_REQ_ECHO:
>  		queue_complete_req(req);
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  
>  	case RAW1394_REQ_ISO_SEND:
>  		print_old_iso_deprecation();
> @@ -2334,24 +2334,24 @@ static int state_connected(struct file_i
>  	case RAW1394_REQ_ISO_LISTEN:
>  		print_old_iso_deprecation();
>  		handle_iso_listen(fi, req);
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  
>  	case RAW1394_REQ_FCP_LISTEN:
>  		handle_fcp_listen(fi, req);
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  
>  	case RAW1394_REQ_RESET_BUS:
>  		if (req->req.misc == RAW1394_LONG_RESET) {
>  			DBGMSG("busreset called (type: LONG)");
>  			hpsb_reset_bus(fi->host, LONG_RESET);
>  			free_pending_request(req);	/* we have to free the request, because we queue no response, and therefore nobody will free it */
> -			return sizeof(struct raw1394_request);
> +			return 0;
>  		}
>  		if (req->req.misc == RAW1394_SHORT_RESET) {
>  			DBGMSG("busreset called (type: SHORT)");
>  			hpsb_reset_bus(fi->host, SHORT_RESET);
>  			free_pending_request(req);	/* we have to free the request, because we queue no response, and therefore nobody will free it */
> -			return sizeof(struct raw1394_request);
> +			return 0;
>  		}
>  		/* error EINVAL (22) invalid argument */
>  		return (-EINVAL);
> @@ -2370,7 +2370,7 @@ static int state_connected(struct file_i
>  		req->req.generation = get_hpsb_generation(fi->host);
>  		req->req.length = 0;
>  		queue_complete_req(req);
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  	}
>  
>  	switch (req->req.type) {
> @@ -2383,7 +2383,7 @@ static int state_connected(struct file_i
>  	if (req->req.length == 0) {
>  		req->req.error = RAW1394_ERROR_INVALID_ARG;
>  		queue_complete_req(req);
> -		return sizeof(struct raw1394_request);
> +		return 0;
>  	}
>  
>  	return handle_async_request(fi, req, node);
> @@ -2394,7 +2394,7 @@ static ssize_t raw1394_write(struct file
>  {
>  	struct file_info *fi = (struct file_info *)file->private_data;
>  	struct pending_request *req;
> -	ssize_t retval = 0;
> +	ssize_t retval = -EBADFD;
>  
>  #ifdef CONFIG_COMPAT
>  	if (count == sizeof(struct compat_raw1394_req) &&
> @@ -2436,6 +2436,9 @@ static ssize_t raw1394_write(struct file
>  
>  	if (retval < 0) {
>  		free_pending_request(req);
> +	} else {
> +		BUG_ON(retval);
> +		retval = count;
>  	}
>  
>  	return retval;
> @@ -2801,6 +2804,99 @@ static int raw1394_ioctl(struct inode *i
>  	return -EINVAL;
>  }
>  
> +#ifdef CONFIG_COMPAT
> +struct raw1394_iso_packets32 {
> +        __u32 n_packets;
> +        compat_uptr_t infos;
> +} __attribute__((packed));
> +
> +struct raw1394_cycle_timer32 {
> +        __u32 cycle_timer;
> +        __u64 local_time;
> +} __attribute__((packed));
> +
> +#define RAW1394_IOC_ISO_RECV_PACKETS32          \
> +        _IOW ('#', 0x25, struct raw1394_iso_packets32)
> +#define RAW1394_IOC_ISO_XMIT_PACKETS32          \
> +        _IOW ('#', 0x27, struct raw1394_iso_packets32)
> +#define RAW1394_IOC_GET_CYCLE_TIMER32           \
> +        _IOR ('#', 0x30, struct raw1394_cycle_timer32)
> +	
> +static long raw1394_iso_xmit_recv_packets32(struct file *file, unsigned int cmd,
> +                                          struct raw1394_iso_packets32 __user *arg)
> +{
> +	compat_uptr_t infos32;
> +	void *infos;
> +	long err = -EFAULT;
> +	struct raw1394_iso_packets __user *dst = compat_alloc_user_space(sizeof(struct raw1394_iso_packets));
> +			
> +	if (!copy_in_user(&dst->n_packets, &arg->n_packets, sizeof arg->n_packets) &&
> +	    !copy_from_user(&infos32, &arg->infos, sizeof infos32)) {
> +		infos = compat_ptr(infos32);
> +		if (!copy_to_user(&dst->infos, &infos, sizeof infos))
> +			err = raw1394_ioctl(NULL, file, cmd, (unsigned long)dst);
> +	}
> +	return err;
> +}
> +
> +static long raw1394_read_cycle_timer32(struct file_info *fi, void __user * uaddr)
> +{
> +	struct raw1394_cycle_timer32 ct;
> +	int err;
> +
> +	err = hpsb_read_cycle_timer(fi->host, &ct.cycle_timer, &ct.local_time);
> +	if (!err)
> +		if (copy_to_user(uaddr, &ct, sizeof(ct)))
> +			err = -EFAULT;
> +	return err;
> +}
> +
> +static long raw1394_compat_ioctl(struct file *file,
> +				 unsigned int cmd, unsigned long arg)
> +{
> +	struct file_info *fi = file->private_data;
> +	void __user *argp = (void __user *)arg;
> +	long err;
> +
> +	lock_kernel();
> +	switch (cmd) {
> +	/* These requests have same format as long as 'int' has same size. */
> +	case RAW1394_IOC_ISO_RECV_INIT:
> +	case RAW1394_IOC_ISO_RECV_START:
> +	case RAW1394_IOC_ISO_RECV_LISTEN_CHANNEL:
> +	case RAW1394_IOC_ISO_RECV_UNLISTEN_CHANNEL:
> +	case RAW1394_IOC_ISO_RECV_SET_CHANNEL_MASK:
> +	case RAW1394_IOC_ISO_RECV_RELEASE_PACKETS:
> +	case RAW1394_IOC_ISO_RECV_FLUSH:
> +	case RAW1394_IOC_ISO_XMIT_RECV_STOP:
> +	case RAW1394_IOC_ISO_XMIT_INIT:
> +	case RAW1394_IOC_ISO_XMIT_START:
> +	case RAW1394_IOC_ISO_XMIT_SYNC:
> +	case RAW1394_IOC_ISO_GET_STATUS:
> +	case RAW1394_IOC_ISO_SHUTDOWN:
> +	case RAW1394_IOC_ISO_QUEUE_ACTIVITY:
> +		err = raw1394_ioctl(NULL, file, cmd, arg);
> +		break;
> +	/* These request have different format. */
> +	case RAW1394_IOC_ISO_RECV_PACKETS32:
> +		err = raw1394_iso_xmit_recv_packets32(file, RAW1394_IOC_ISO_RECV_PACKETS, argp);
> +		break;
> +	case RAW1394_IOC_ISO_XMIT_PACKETS32:
> +		err = raw1394_iso_xmit_recv_packets32(file, RAW1394_IOC_ISO_XMIT_PACKETS, argp);
> +		break;
> +	case RAW1394_IOC_GET_CYCLE_TIMER32:
> +		err = raw1394_read_cycle_timer32(fi, argp);
> +		break;
> +	default:
> +		err = -EINVAL;
> +		break;
> +	}
> +	unlock_kernel();
> +
> +	return err;
> +}
> +#endif
> +
>  static unsigned int raw1394_poll(struct file *file, poll_table * pt)
>  {
>  	struct file_info *fi = file->private_data;
> @@ -3040,7 +3136,9 @@ static const struct file_operations raw1
>  	.write = raw1394_write,
>  	.mmap = raw1394_mmap,
>  	.ioctl = raw1394_ioctl,
> -	// .compat_ioctl = ... someone needs to do this
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl = raw1394_compat_ioctl,
> +#endif
>  	.poll = raw1394_poll,
>  	.open = raw1394_open,
>  	.release = raw1394_release,
> 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/