On Tue, 5 Aug 2008, Sven Wegener wrote:

quoted text
> On Tue, 5 Aug 2008, Simon Horman wrote:
> 
> > Check the return value of device_create_bin_file in pci_create_bus,
> > unwind if neccessary, and propogate any errors to the caller.
> > 
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> > 
> > --- 
> > 
> > drivers/pci/probe.c: In function `pci_create_bus':
> > drivers/pci/probe.c:66: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
> > drivers/pci/probe.c:74: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
> > 
> > # ia64-unknown-linux-gnu-gcc --version
> > ia64-unknown-linux-gnu-gcc (GCC) 3.4.5
> > Copyright (C) 2004 Free Software Foundation, Inc.
> > This is free software; see the source for copying conditions.  There is NO
> > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> > 
> > Index: linux-2.6/drivers/pci/probe.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/pci/probe.c	2008-08-05 19:58:43.000000000 +1000
> > +++ linux-2.6/drivers/pci/probe.c	2008-08-05 19:59:15.000000000 +1000
> > @@ -53,26 +53,37 @@ EXPORT_SYMBOL(no_pci_devices);
> >   * a per-bus basis.  This routine creates the files and ties them into
> >   * their associated read, write and mmap files from pci-sysfs.c
> >   */
> > -static void pci_create_legacy_files(struct pci_bus *b)
> > +static int pci_create_legacy_files(struct pci_bus *b)
> >  {
> > +	int error;
> > +
> >  	b->legacy_io = kzalloc(sizeof(struct bin_attribute) * 2,
> >  			       GFP_ATOMIC);
> > -	if (b->legacy_io) {
> > -		b->legacy_io->attr.name = "legacy_io";
> > -		b->legacy_io->size = 0xffff;
> > -		b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
> > -		b->legacy_io->read = pci_read_legacy_io;
> > -		b->legacy_io->write = pci_write_legacy_io;
> > -		device_create_bin_file(&b->dev, b->legacy_io);
> > -
> > -		/* Allocated above after the legacy_io struct */
> > -		b->legacy_mem = b->legacy_io + 1;
> > -		b->legacy_mem->attr.name = "legacy_mem";
> > -		b->legacy_mem->size = 1024*1024;
> > -		b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
> > -		b->legacy_mem->mmap = pci_mmap_legacy_mem;
> > -		device_create_bin_file(&b->dev, b->legacy_mem);
> > +	if (!b->legacy_io)
> > +		return -ENOMEM;
> > +
> > +	b->legacy_io->attr.name = "legacy_io";
> > +	b->legacy_io->size = 0xffff;
> > +	b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
> > +	b->legacy_io->read = pci_read_legacy_io;
> > +	b->legacy_io->write = pci_write_legacy_io;
> > +	error = device_create_bin_file(&b->dev, b->legacy_io);
> > +	if (error)
> > +		return error;
> 
> I'd release the memory here and NULLify legacy_io.
> 
> > +
> > +	/* Allocated above after the legacy_io struct */
> > +	b->legacy_mem = b->legacy_io + 1;
> > +	b->legacy_mem->attr.name = "legacy_mem";
> > +	b->legacy_mem->size = 1024*1024;
> > +	b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
> > +	b->legacy_mem->mmap = pci_mmap_legacy_mem;
> > +	error = device_create_bin_file(&b->dev, b->legacy_mem);
> > +	if (error) {
> > +		device_remove_bin_file(&b->dev, b->legacy_io);
> > +		return error;
> 
> Here too.
> 
> Reason: If we fail to create the legacy_io file, legacy_mem will still be 
> NULL, because it has been not initialized at that point. But we will try 
> to remove it in pci_remove_legacy_files and in sysfs_remove_bin_file 
> we're going to derefence it and blow up.

Uhm, this case can not happen in this version of the patch. We'll actually 
fail registering the bus completely with this patch, so there should be no 
chance that we'll ever go through pci_remove_legacy_files with legacy_io 
!= NULL and legacy_mem == NULL. So no need to NULLify it. But with the 
change Matthew suggested, we need to pay attention to it. Sorry for the 
noise here.

quoted text
> >  	}
> > +
> > +	return 0;
> >  }
> >
> >  void pci_remove_legacy_files(struct pci_bus *b)
> > @@ -84,7 +95,7 @@ void pci_remove_legacy_files(struct pci_
> >  	}
> >  }
> >  #else /* !HAVE_PCI_LEGACY */
> > -static inline void pci_create_legacy_files(struct pci_bus *bus) { return; }
> > +static inline int pci_create_legacy_files(struct pci_bus *bus) { return 0; }
> >  void pci_remove_legacy_files(struct pci_bus *bus) { return; }
> >  #endif /* HAVE_PCI_LEGACY */
> >  
> > @@ -1157,7 +1168,9 @@ struct pci_bus * pci_create_bus(struct d
> >  		goto dev_create_file_err;
> >  
> >  	/* Create legacy_io and legacy_mem files for this bus */
> > -	pci_create_legacy_files(b);
> > +	error = pci_create_legacy_files(b);
> > +	if (error)
> > +		goto pci_create_legacy_files_err;
> >  
> >  	b->number = b->secondary = bus;
> >  	b->resource[0] = &ioport_resource;
> > @@ -1167,6 +1180,8 @@ struct pci_bus * pci_create_bus(struct d
> >  
> >  	return b;
> >  
> > +pci_create_legacy_files_err:
> > +	device_remove_file(&b->dev, &dev_attr_cpuaffinity);
> >  dev_create_file_err:
> >  	device_unregister(&b->dev);
> >  class_dev_reg_err:
--
unsubscribe notice
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/