PATCH: switch ide-proc to use the ide_key functionality

PATCH: switch ide-proc to use the ide_key functionality

[Alan Cox]

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.8-rc3/drivers/ide/ide-proc.c linux-2.6.8-rc3/drivers/ide/ide-proc.c
--- linux.vanilla-2.6.8-rc3/drivers/ide/ide-proc.c	2004-08-09 15:51:00.000000000 +0100
+++ linux-2.6.8-rc3/drivers/ide/ide-proc.c	2004-08-12 17:26:00.000000000 +0100
@@ -355,7 +355,7 @@
 static int proc_ide_read_identify
 	(char *page, char **start, off_t off, int count, int *eof, void *data)
 {
-	ide_drive_t	*drive = (ide_drive_t *)data;
+	ide_drive_t	*drive = ide_drive_from_key(data);
 	int		len = 0, i = 0;
 	int		err = 0;
 
@@ -397,11 +397,15 @@
 static int proc_ide_read_settings
 	(char *page, char **start, off_t off, int count, int *eof, void *data)
 {
-	ide_drive_t	*drive = (ide_drive_t *) data;
-	ide_settings_t	*setting = (ide_settings_t *) drive->settings;
+	ide_drive_t	*drive = ide_drive_from_key(data);
+	ide_settings_t	*setting;
 	char		*out = page;
 	int		len, rc, mul_factor, div_factor;
+	
+	if(drive == NULL)
+		return -EIO;
 
+	setting = (ide_settings_t *) drive->settings;
 	down(&ide_setting_sem);
 	out += sprintf(out, "name\t\t\tvalue\t\tmin\t\tmax\t\tmode\n");
 	out += sprintf(out, "----\t\t\t-----\t\t---\t\t---\t\t----\n");
@@ -431,7 +435,7 @@
 static int proc_ide_write_settings(struct file *file, const char __user *buffer,
 				   unsigned long count, void *data)
 {
-	ide_drive_t	*drive = (ide_drive_t *) data;
+	ide_drive_t	*drive = ide_drive_from_key(data);
 	char		name[MAX_LEN + 1];
 	int		for_real = 0;
 	unsigned long	n;
@@ -440,6 +444,9 @@
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
+		
+	if (drive == NULL)
+		return -EIO;
 
 	if (count >= PAGE_SIZE)
 		return -EINVAL;
@@ -523,9 +530,12 @@
 int proc_ide_read_capacity
 	(char *page, char **start, off_t off, int count, int *eof, void *data)
 {
-	ide_drive_t	*drive = (ide_drive_t *) data;
+	ide_drive_t	*drive = ide_drive_from_key(data);
 	int		len;
 
+	if(drive == NULL)
+		return -EIO;
+		
 	len = sprintf(page,"%llu\n",
 		      (long long) (DRIVER(drive)->capacity(drive)));
 	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
@@ -534,9 +544,12 @@
 int proc_ide_read_geometry
 	(char *page, char **start, off_t off, int count, int *eof, void *data)
 {
-	ide_drive_t	*drive = (ide_drive_t *) data;
+	ide_drive_t	*drive = ide_drive_from_key(data);
 	char		*out = page;
 	int		len;
+	
+	if(drive == NULL)
+		return -EIO;
 
 	out += sprintf(out,"physical     %d/%d/%d\n",
 			drive->cyl, drive->head, drive->sect);
@@ -552,10 +565,14 @@
 static int proc_ide_read_dmodel
 	(char *page, char **start, off_t off, int count, int *eof, void *data)
 {
-	ide_drive_t	*drive = (ide_drive_t *) data;
-	struct hd_driveid *id = drive->id;
+	ide_drive_t	*drive = ide_drive_from_key(data);
+	struct hd_driveid *id;
 	int		len;
 
+	if(drive == NULL)
+		return -EIO;
+
+	id = drive->id;
 	len = sprintf(page, "%.40s\n",
 		(id && id->model[0]) ? (char *)id->model : "(none)");
 	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
@@ -564,40 +581,30 @@
 static int proc_ide_read_driver
 	(char *page, char **start, off_t off, int count, int *eof, void *data)
 {
-	ide_drive_t	*drive = (ide_drive_t *) data;
-	ide_driver_t	*driver = drive->driver;
+	ide_drive_t	*drive = ide_drive_from_key(data);
+	ide_driver_t	*driver;
 	int		len;
 
+	if(drive == NULL)
+		return -EIO;
+			
+	driver = drive->driver;
+
 	len = sprintf(page, "%s version %s\n",
 			driver->name, driver->version);
 	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
 }
 
-static int proc_ide_write_driver
-	(struct file *file, const char __user *buffer, unsigned long count, void *data)
-{
-	ide_drive_t	*drive = (ide_drive_t *) data;
-	char name[32];
-
-	if (!capable(CAP_SYS_ADMIN))
-		return -EACCES;
-	if (count > 31)
-		count = 31;
-	if (copy_from_user(name, buffer, count))
-		return -EFAULT;
-	name[count] = '\0';
-	if (ide_replace_subdriver(drive, name))
-		return -EINVAL;
-	return count;
-}
-
 static int proc_ide_read_media
 	(char *page, char **start, off_t off, int count, int *eof, void *data)
 {
-	ide_drive_t	*drive = (ide_drive_t *) data;
+	ide_drive_t	*drive = ide_drive_from_key(data);
 	const char	*media;
 	int		len;
 
+	if(drive == NULL)
+		return -EINVAL;
+
 	switch (drive->media) {
 		case ide_disk:	media = "disk\n";
 				break;
@@ -616,7 +623,7 @@
 }
 
 static ide_proc_entry_t generic_drive_entries[] = {
-	{ "driver",	S_IFREG|S_IRUGO,	proc_ide_read_driver,	proc_ide_write_driver },
+	{ "driver",	S_IFREG|S_IRUGO,	proc_ide_read_driver,	NULL },
 	{ "identify",	S_IFREG|S_IRUSR,	proc_ide_read_identify,	NULL },
 	{ "media",	S_IFREG|S_IRUGO,	proc_ide_read_media,	NULL },
 	{ "model",	S_IFREG|S_IRUGO,	proc_ide_read_dmodel,	NULL },
@@ -668,7 +675,7 @@
 
 		drive->proc = proc_mkdir(drive->name, parent);
 		if (drive->proc)
-			ide_add_proc_entries(drive->proc, generic_drive_entries, drive);
+			ide_add_proc_entries(drive->proc, generic_drive_entries, ide_drive_to_key(drive));
 		sprintf(name,"ide%d/%s", (drive->name[2]-'a')/2, drive->name);
 		ent = proc_symlink(drive->name, proc_ide_root, name);
 		if (!ent) return;


Signed-off-by: Alan Cox <alan@redhat.com>

Re: PATCH: switch ide-proc to use the ide_key functionality

[Bartlomiej Zolnierkiewicz]

This patch removes write access to /proc/ide/hd?/driver without even
mentioning this - IMHO we should deprecate it first.  Such changes should
be described (with rationale for them) at least in the changelog
(or even better in Documentation/ide.txt).

On Sunday 15 August 2004 17:04, Alan Cox wrote:
> diff -u --new-file --recursive --exclude-from /usr/src/exclude
> linux.vanilla-2.6.8-rc3/drivers/ide/ide-proc.c
> linux-2.6.8-rc3/drivers/ide/ide-proc.c ---
> linux.vanilla-2.6.8-rc3/drivers/ide/ide-proc.c	2004-08-09
> 15:51:00.000000000 +0100 +++
> linux-2.6.8-rc3/drivers/ide/ide-proc.c	2004-08-12 17:26:00.000000000 +0100
> @@ -355,7 +355,7 @@
>  static int proc_ide_read_identify
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *)data;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
>  	int		len = 0, i = 0;
>  	int		err = 0;
>
> @@ -397,11 +397,15 @@
>  static int proc_ide_read_settings
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> -	ide_settings_t	*setting = (ide_settings_t *) drive->settings;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
> +	ide_settings_t	*setting;
>  	char		*out = page;
>  	int		len, rc, mul_factor, div_factor;
> +
> +	if(drive == NULL)
> +		return -EIO;
>
> +	setting = (ide_settings_t *) drive->settings;
>  	down(&ide_setting_sem);
>  	out += sprintf(out, "name\t\t\tvalue\t\tmin\t\tmax\t\tmode\n");
>  	out += sprintf(out, "----\t\t\t-----\t\t---\t\t---\t\t----\n");
> @@ -431,7 +435,7 @@
>  static int proc_ide_write_settings(struct file *file, const char __user
> *buffer, unsigned long count, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
>  	char		name[MAX_LEN + 1];
>  	int		for_real = 0;
>  	unsigned long	n;
> @@ -440,6 +444,9 @@
>
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EACCES;
> +
> +	if (drive == NULL)
> +		return -EIO;
>
>  	if (count >= PAGE_SIZE)
>  		return -EINVAL;
> @@ -523,9 +530,12 @@
>  int proc_ide_read_capacity
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
>  	int		len;
>
> +	if(drive == NULL)
> +		return -EIO;
> +
>  	len = sprintf(page,"%llu\n",
>  		      (long long) (DRIVER(drive)->capacity(drive)));
>  	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
> @@ -534,9 +544,12 @@
>  int proc_ide_read_geometry
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
>  	char		*out = page;
>  	int		len;
> +
> +	if(drive == NULL)
> +		return -EIO;
>
>  	out += sprintf(out,"physical     %d/%d/%d\n",
>  			drive->cyl, drive->head, drive->sect);
> @@ -552,10 +565,14 @@
>  static int proc_ide_read_dmodel
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> -	struct hd_driveid *id = drive->id;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
> +	struct hd_driveid *id;
>  	int		len;
>
> +	if(drive == NULL)
> +		return -EIO;
> +
> +	id = drive->id;
>  	len = sprintf(page, "%.40s\n",
>  		(id && id->model[0]) ? (char *)id->model : "(none)");
>  	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
> @@ -564,40 +581,30 @@
>  static int proc_ide_read_driver
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> -	ide_driver_t	*driver = drive->driver;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
> +	ide_driver_t	*driver;
>  	int		len;
>
> +	if(drive == NULL)
> +		return -EIO;
> +
> +	driver = drive->driver;
> +
>  	len = sprintf(page, "%s version %s\n",
>  			driver->name, driver->version);
>  	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
>  }
>
> -static int proc_ide_write_driver
> -	(struct file *file, const char __user *buffer, unsigned long count, void
> *data) -{
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> -	char name[32];
> -
> -	if (!capable(CAP_SYS_ADMIN))
> -		return -EACCES;
> -	if (count > 31)
> -		count = 31;
> -	if (copy_from_user(name, buffer, count))
> -		return -EFAULT;
> -	name[count] = '\0';
> -	if (ide_replace_subdriver(drive, name))
> -		return -EINVAL;
> -	return count;
> -}
> -
>  static int proc_ide_read_media
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
>  	const char	*media;
>  	int		len;
>
> +	if(drive == NULL)
> +		return -EINVAL;
> +
>  	switch (drive->media) {
>  		case ide_disk:	media = "disk\n";
>  				break;
> @@ -616,7 +623,7 @@
>  }
>
>  static ide_proc_entry_t generic_drive_entries[] = {
> -	{ "driver",	S_IFREG|S_IRUGO,	proc_ide_read_driver,	proc_ide_write_driver
> }, +	{ "driver",	S_IFREG|S_IRUGO,	proc_ide_read_driver,	NULL },
>  	{ "identify",	S_IFREG|S_IRUSR,	proc_ide_read_identify,	NULL },
>  	{ "media",	S_IFREG|S_IRUGO,	proc_ide_read_media,	NULL },
>  	{ "model",	S_IFREG|S_IRUGO,	proc_ide_read_dmodel,	NULL },
> @@ -668,7 +675,7 @@
>
>  		drive->proc = proc_mkdir(drive->name, parent);
>  		if (drive->proc)
> -			ide_add_proc_entries(drive->proc, generic_drive_entries, drive);
> +			ide_add_proc_entries(drive->proc, generic_drive_entries,
> ide_drive_to_key(drive)); sprintf(name,"ide%d/%s", (drive->name[2]-'a')/2,
> drive->name);
>  		ent = proc_symlink(drive->name, proc_ide_root, name);
>  		if (!ent) return;

Re: PATCH: switch ide-proc to use the ide_key functionality

[Alan Cox]

On Mon, Aug 16, 2004 at 05:24:09PM +0200, Bartlomiej Zolnierkiewicz wrote:
> This patch removes write access to /proc/ide/hd?/driver without even
> mentioning this - IMHO we should deprecate it first.  Such changes should
> be described (with rationale for them) at least in the changelog
> (or even better in Documentation/ide.txt).

I thought we discussed this earlier - its essentially unfixable. You can
either have trivial /proc crashes, deadlocks on writing this file or lose
the feature (which nobody on the planet even knew about). 

If you've got any ideas how to fix it then let me know.

Agreed about adding it to Documentation/ide.txt

Re: PATCH: switch ide-proc to use the ide_key functionality

[Bartlomiej Zolnierkiewicz]

On Monday 16 August 2004 17:30, Alan Cox wrote:
> On Mon, Aug 16, 2004 at 05:24:09PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > This patch removes write access to /proc/ide/hd?/driver without even
> > mentioning this - IMHO we should deprecate it first.  Such changes should
> > be described (with rationale for them) at least in the changelog
> > (or even better in Documentation/ide.txt).
>
> I thought we discussed this earlier - its essentially unfixable. You can
> either have trivial /proc crashes, deadlocks on writing this file or lose
> the feature (which nobody on the planet even knew about).

I used this 'feature' few times and never experienced crash or deadlock
but they are possible of course.

> If you've got any ideas how to fix it then let me know.
>
> Agreed about adding it to Documentation/ide.txt

I fully agreed on the change but not the way in which it's done. ;-)

Re: PATCH: switch ide-proc to use the ide_key functionality

[Bartlomiej Zolnierkiewicz]

Alan, 'ide_key' protection misses driver specfiic /proc/entries.

It is also still racy for some drivers because ide_register_hw() -> 
init_hwif_data() sets hwif->key to zero - you must set hwif->hold to 1.

[ There might be other similar problems, need to double check. ]

Can't we solve the problem in simpler way by covering affected /proc
handlers with ide_setting_sem?

On Sunday 15 August 2004 17:04, Alan Cox wrote:
> diff -u --new-file --recursive --exclude-from /usr/src/exclude
> linux.vanilla-2.6.8-rc3/drivers/ide/ide-proc.c
> linux-2.6.8-rc3/drivers/ide/ide-proc.c ---
> linux.vanilla-2.6.8-rc3/drivers/ide/ide-proc.c	2004-08-09
> 15:51:00.000000000 +0100 +++
> linux-2.6.8-rc3/drivers/ide/ide-proc.c	2004-08-12 17:26:00.000000000 +0100
> @@ -355,7 +355,7 @@
>  static int proc_ide_read_identify
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *)data;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
>  	int		len = 0, i = 0;
>  	int		err = 0;
>
> @@ -397,11 +397,15 @@
>  static int proc_ide_read_settings
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> -	ide_settings_t	*setting = (ide_settings_t *) drive->settings;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
> +	ide_settings_t	*setting;
>  	char		*out = page;
>  	int		len, rc, mul_factor, div_factor;
> +
> +	if(drive == NULL)
> +		return -EIO;
>
> +	setting = (ide_settings_t *) drive->settings;
>  	down(&ide_setting_sem);
>  	out += sprintf(out, "name\t\t\tvalue\t\tmin\t\tmax\t\tmode\n");
>  	out += sprintf(out, "----\t\t\t-----\t\t---\t\t---\t\t----\n");
> @@ -431,7 +435,7 @@
>  static int proc_ide_write_settings(struct file *file, const char __user
> *buffer, unsigned long count, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
>  	char		name[MAX_LEN + 1];
>  	int		for_real = 0;
>  	unsigned long	n;
> @@ -440,6 +444,9 @@
>
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EACCES;
> +
> +	if (drive == NULL)
> +		return -EIO;
>
>  	if (count >= PAGE_SIZE)
>  		return -EINVAL;
> @@ -523,9 +530,12 @@
>  int proc_ide_read_capacity
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
>  	int		len;
>
> +	if(drive == NULL)
> +		return -EIO;
> +
>  	len = sprintf(page,"%llu\n",
>  		      (long long) (DRIVER(drive)->capacity(drive)));
>  	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
> @@ -534,9 +544,12 @@
>  int proc_ide_read_geometry
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
>  	char		*out = page;
>  	int		len;
> +
> +	if(drive == NULL)
> +		return -EIO;
>
>  	out += sprintf(out,"physical     %d/%d/%d\n",
>  			drive->cyl, drive->head, drive->sect);
> @@ -552,10 +565,14 @@
>  static int proc_ide_read_dmodel
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> -	struct hd_driveid *id = drive->id;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
> +	struct hd_driveid *id;
>  	int		len;
>
> +	if(drive == NULL)
> +		return -EIO;
> +
> +	id = drive->id;
>  	len = sprintf(page, "%.40s\n",
>  		(id && id->model[0]) ? (char *)id->model : "(none)");
>  	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
> @@ -564,40 +581,30 @@
>  static int proc_ide_read_driver
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> -	ide_driver_t	*driver = drive->driver;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
> +	ide_driver_t	*driver;
>  	int		len;
>
> +	if(drive == NULL)
> +		return -EIO;
> +
> +	driver = drive->driver;
> +
>  	len = sprintf(page, "%s version %s\n",
>  			driver->name, driver->version);
>  	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
>  }
>
> -static int proc_ide_write_driver
> -	(struct file *file, const char __user *buffer, unsigned long count, void
> *data) -{
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> -	char name[32];
> -
> -	if (!capable(CAP_SYS_ADMIN))
> -		return -EACCES;
> -	if (count > 31)
> -		count = 31;
> -	if (copy_from_user(name, buffer, count))
> -		return -EFAULT;
> -	name[count] = '\0';
> -	if (ide_replace_subdriver(drive, name))
> -		return -EINVAL;
> -	return count;
> -}
> -
>  static int proc_ide_read_media
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
> -	ide_drive_t	*drive = (ide_drive_t *) data;
> +	ide_drive_t	*drive = ide_drive_from_key(data);
>  	const char	*media;
>  	int		len;
>
> +	if(drive == NULL)
> +		return -EINVAL;
> +
>  	switch (drive->media) {
>  		case ide_disk:	media = "disk\n";
>  				break;
> @@ -616,7 +623,7 @@
>  }
>
>  static ide_proc_entry_t generic_drive_entries[] = {
> -	{ "driver",	S_IFREG|S_IRUGO,	proc_ide_read_driver,	proc_ide_write_driver
> }, +	{ "driver",	S_IFREG|S_IRUGO,	proc_ide_read_driver,	NULL },
>  	{ "identify",	S_IFREG|S_IRUSR,	proc_ide_read_identify,	NULL },
>  	{ "media",	S_IFREG|S_IRUGO,	proc_ide_read_media,	NULL },
>  	{ "model",	S_IFREG|S_IRUGO,	proc_ide_read_dmodel,	NULL },
> @@ -668,7 +675,7 @@
>
>  		drive->proc = proc_mkdir(drive->name, parent);
>  		if (drive->proc)
> -			ide_add_proc_entries(drive->proc, generic_drive_entries, drive);
> +			ide_add_proc_entries(drive->proc, generic_drive_entries,
> ide_drive_to_key(drive)); sprintf(name,"ide%d/%s", (drive->name[2]-'a')/2,
> drive->name);
>  		ent = proc_symlink(drive->name, proc_ide_root, name);
>  		if (!ent) return;
>
>
> Signed-off-by: Alan Cox <alan@redhat.com>

Re: PATCH: switch ide-proc to use the ide_key functionality

[Alan Cox]

On Tue, Aug 17, 2004 at 01:35:11AM +0200, Bartlomiej Zolnierkiewicz wrote:
> Alan, 'ide_key' protection misses driver specfiic /proc/entries.

Ok need to fix that

> It is also still racy for some drivers because ide_register_hw() -> 
> init_hwif_data() sets hwif->key to zero - you must set hwif->hold to 1.

ide_register_hw holds ide_setting_sem. I think that should be ok ?

> Can't we solve the problem in simpler way by covering affected /proc
> handlers with ide_setting_sem?

You'd need to refcount the ide objects because the 'data' value is long
lived. I did consider this but it seemed more complex. It also leaves end
users able to open an ide file and prevent unloading although I'm not
sure it is a big issue.

Re: PATCH: switch ide-proc to use the ide_key functionality

[Bartlomiej Zolnierkiewicz]

On Tuesday 17 August 2004 02:13, Alan Cox wrote:
> On Tue, Aug 17, 2004 at 01:35:11AM +0200, Bartlomiej Zolnierkiewicz wrote:
> > Alan, 'ide_key' protection misses driver specfiic /proc/entries.
>
> Ok need to fix that
>
> > It is also still racy for some drivers because ide_register_hw() ->
> > init_hwif_data() sets hwif->key to zero - you must set hwif->hold to 1.
>
> ide_register_hw holds ide_setting_sem. I think that should be ok ?

ide_setting_sem doesn't help situation when hwif is unregistered and some 
other driver is loaded later and takes this hwif using ide_register_hw().

Highly unlikely but possible.

> > Can't we solve the problem in simpler way by covering affected /proc
> > handlers with ide_setting_sem?
>
> You'd need to refcount the ide objects because the 'data' value is long
> lived. I did consider this but it seemed more complex. It also leaves end
> users able to open an ide file and prevent unloading although I'm not
> sure it is a big issue.

Yep. :/

Re: PATCH: switch ide-proc to use the ide_key functionality

[Alan Cox]

On Tue, Aug 17, 2004 at 02:31:25AM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > It is also still racy for some drivers because ide_register_hw() ->
> > > init_hwif_data() sets hwif->key to zero - you must set hwif->hold to 1.
> >
> > ide_register_hw holds ide_setting_sem. I think that should be ok ?
> 
> ide_setting_sem doesn't help situation when hwif is unregistered and some 
> other driver is loaded later and takes this hwif using ide_register_hw().

Right - ide_cfg_sem is covering this. ide_cfg_sem is taken after 
ide_setting_sem so how about making init_hwif_data restore that field.

ide_drive_from_key can then take ide_cfg_sem during its checking

The other alternative is to never use key = 0 in the real world but I
see no reason for not just taking ide_cfg_sem during the key to drive
operation. ide_cfg_sem is the read lock for configuration change so this
is logically the right behaviour too ?

Re: PATCH: switch ide-proc to use the ide_key functionality

[Alan Cox]

BTW this may help if I document the locks and what they cover in case its
not obvious

drives_lock is the spin lock you must own to update the drive list

ide_cfg_sem is the semaphore you must own to walk or hold loose references
to the ide_hwif_t array and elements relating to adding/removing/busy status

ide_lock is taken when you want to write that array and deal with elements
that are interrupt walked by other devices. Notably this means the hwgroup
chains.

ide_settings_sem could be per drive but isnt, it is used when you are
processing the ioctl/proc objects attached to the driver either by walking
them or removing them.


drivers_lock is the spin lock you must own to update the drivers list
drivers_sem is the semaphore you must own to walk the list and while
holding loose references to drivers (ie when the busy/lists are not
consistent)

The lock order is

	ide_cfg_sem
	drivers_sem
	ide_settings_sem
	drivers_lock | drives_lock
	ide_lock


Which means my cunning plan from the previous mail doesn't actually work
unless we take ide_cfg_sem at the top of the proc code before setting_sem. 
Also looking over it I need to send you the bits to take the sems in each 
proc routine for that case.  

PS: what do you think about deprecating (but not yet removing) ide_write_config
and ide_read_config now we have sysfs ? Does anyone actually use its
"wait for non busy and then pci config write" - does anyone use it at all ?

Also while looking at proc it would clean up read_imodel and make it a ton
more useful to stick the string pointers into hwif->chipset_name or somesuch
so we can report the PCI ones in detail ?

Alan

Re: PATCH: switch ide-proc to use the ide_key functionality

[Bartlomiej Zolnierkiewicz]

On Tuesday 17 August 2004 03:05, Alan Cox wrote:
> BTW this may help if I document the locks and what they cover in case its
> not obvious
>
> drives_lock is the spin lock you must own to update the drive list
>
> ide_cfg_sem is the semaphore you must own to walk or hold loose references
> to the ide_hwif_t array and elements relating to adding/removing/busy
> status
>
> ide_lock is taken when you want to write that array and deal with elements
> that are interrupt walked by other devices. Notably this means the hwgroup
> chains.
>
> ide_settings_sem could be per drive but isnt, it is used when you are
> processing the ioctl/proc objects attached to the driver either by walking
> them or removing them.
>
>
> drivers_lock is the spin lock you must own to update the drivers list
> drivers_sem is the semaphore you must own to walk the list and while
> holding loose references to drivers (ie when the busy/lists are not
> consistent)
>
> The lock order is
>
> 	ide_cfg_sem
> 	drivers_sem
> 	ide_settings_sem
> 	drivers_lock | drives_lock
> 	ide_lock
>
>
> Which means my cunning plan from the previous mail doesn't actually work
> unless we take ide_cfg_sem at the top of the proc code before setting_sem.
> Also looking over it I need to send you the bits to take the sems in each
> proc routine for that case.

Yes, on the other hand we may try to do real refcounting.

Actually I think there are holes in the above locking scheme but need to think 
more about it - i.e. what protects us from removing hwif while it is being 
configured by host driver?

> PS: what do you think about deprecating (but not yet removing)
> ide_write_config and ide_read_config now we have sysfs ? Does anyone

sysfs doesn't help at all as IDE is not ready for full sysfs support
(missed locking, no refcounting and no dynamic objects)

I also can't see how sysfs can help with synchronizing writes to ISA/PCI 
config space with ongoing I/O?

Please also note that PCI writes currently doesn't work because of "... || 
hwif->pci_dev->vendor" instead of "... || !hwif->pci_dev->vendor" - somebody 
(I think you :) broke this back in 2.4.21 kernel.

> actually use its "wait for non busy and then pci config write" - does
> anyone use it at all ?

I'm not aware of any users and I have patch ready to deprecate it.

> Also while looking at proc it would clean up read_imodel and make it a ton
> more useful to stick the string pointers into hwif->chipset_name or
> somesuch so we can report the PCI ones in detail ?

Yes, but reporting PCI ones in detail will brake any users of this /proc entry 
(if there are any of course).

Thanks.

Re: PATCH: switch ide-proc to use the ide_key functionality

[Alan Cox]

On Tue, Aug 17, 2004 at 12:48:12PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > Which means my cunning plan from the previous mail doesn't actually work
> > unless we take ide_cfg_sem at the top of the proc code before setting_sem.
> > Also looking over it I need to send you the bits to take the sems in each
> > proc routine for that case.
> 
> Yes, on the other hand we may try to do real refcounting.

refcounting doesn't solve the need for keys however due to the way procfs works
I've fixed the keys in my tree by taking the cfg/settings sem for all those
proc functions. I've also done it for the hwif level proc functions.

I'll have a look at what occurs if we make the ->key functions ref count
and add "put" functions. I think that can be made to work cleanly without
changing the rest of the code to refcounts at the same time. It'll still need
some locking because of the memset.  We would still have keys but we'd
refcount usage off them as a starting point.

> more about it - i.e. what protects us from removing hwif while it is being 
> configured by host driver?

A hwif is only removed by the owner. Thus for the PCI devices it is 
protected by higher level serializing. 

> I also can't see how sysfs can help with synchronizing writes to ISA/PCI 
> config space with ongoing I/O?

The question is do we need it ?

Alan

Re: PATCH: switch ide-proc to use the ide_key functionality

[Alan Cox]

On Tue, Aug 17, 2004 at 08:06:22AM -0400, Alan Cox wrote:
> I'll have a look at what occurs if we make the ->key functions ref count
> and add "put" functions. I think that can be made to work cleanly without
> changing the rest of the code to refcounts at the same time. It'll still need
> some locking because of the memset.  We would still have keys but we'd
> refcount usage off them as a starting point.

Ok fixed this by using the cfg_sem. refcounting breaks the non refcounted
code and its assumptions (good bad or otherwise). I've dropped the locking
in in such a way as switching to refcounts later is easier.

Alan