From: Mark Einon <mark.einon@gmail.com> To: Tobias Klauser <tklauser@distanz.ch> Cc: gregkh@linuxfoundation.org, davem@davemloft.net, devel@driverdev.osuosl.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH linux-next] et131x: Promote staging et131x driver to drivers/net Date: Tue, 23 Sep 2014 10:46:15 +0100 [thread overview] Message-ID: <20140923094614.GA5130@leicester.auvation.com> (raw) In-Reply-To: <20140923072251.GD4657@distanz.ch> On Tue, Sep 23, 2014 at 09:22:53AM +0200, Tobias Klauser wrote: Hi Tobias, Thanks for the details review. I've replied below - [...] > > +/* et131x_rx_dma_memory_alloc > > + * > > + * Allocates Free buffer ring 1 for sure, free buffer ring 0 if required, > > + * and the Packet Status Ring. > > + */ > > +static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter) > > +{ > > + u8 id; > > + u32 i, j; > > + u32 bufsize; > > + u32 psr_size; > > + u32 fbr_chunksize; > > + struct rx_ring *rx_ring = &adapter->rx_ring; > > + struct fbr_lookup *fbr; > > + > > + /* Alloc memory for the lookup table */ > > + rx_ring->fbr[0] = kmalloc(sizeof(*fbr), GFP_KERNEL); > > + if (rx_ring->fbr[0] == NULL) > > + return -ENOMEM; > > + rx_ring->fbr[1] = kmalloc(sizeof(*fbr), GFP_KERNEL); > > + if (rx_ring->fbr[1] == NULL) > > + return -ENOMEM; > > If you fail here, et131x_rx_dma_memory_free() will be called by > et131x_adapter_memory_free() in the error handling path which in turn > will access the members of the already allocated rx_ring->fbr[0] (e.g. > ->buffsize). Since it is allocated with kmalloc() these members contain > arbitrary values and cause problems in et131x_rx_dma_memory_free(). I' > suggest to do the cleanup (i.e. free rx_ring->fbr[0] directly here and > not call et131x_rx_dma_memory_free() in the error handling path. You > might want to check that memory allocation failures are properly handled > in the rest of the driver as well. There are multiple other cases where > et131x_adapter_memory_free() is called on partially > allocated/initialized memory. > I don't think this is the case - the members of rx_ring->fbr[x] are not accessed unless this pointer is non-NULL in et131x_rx_dma_memory_free() (see code snippet below), which is exactly why the kmalloc code above would fail, so buffsize never gets accessed and the code is cleaned up correctly. Actually, for further explanation, this thread discusses these changes: http://www.spinics.net/lists/linux-driver-devel/msg42128.html > > +/* et131x_rx_dma_memory_free - Free all memory allocated within this module */ > > +static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter) > > +{ [...] > > + /* Free Free Buffer Rings */ > > + for (id = 0; id < NUM_FBRS; id++) { > > + fbr = rx_ring->fbr[id]; > > + > > + if (!fbr || !fbr->ring_virtaddr) > > + continue; > > + > > + /* First the packet memory */ > > + for (ii = 0; ii < fbr->num_entries / FBR_CHUNKS; ii++) { > > + if (fbr->mem_virtaddrs[ii]) { > > + bufsize = fbr->buffsize * FBR_CHUNKS; [...] > > +static SIMPLE_DEV_PM_OPS(et131x_pm_ops, et131x_suspend, et131x_resume); > > +#define ET131X_PM_OPS (&et131x_pm_ops) > > +#else > > +#define ET131X_PM_OPS NULL > > +#endif > > No need for the #define here, just assigne et131x_pm_ops to .driver.pm > directly, its members will be NULL and thus never called. Also, you can > make et131x_pm_ops const. Ok, I can change this. Btw, this appears to be a fairly standard way of using .driver.pm among ethernet drivers, e.g. see 3com/3c59x.c, atheros, marvell... - perhaps there is a case for changing all instances of this code? > > + > > +/* et131x_isr - The Interrupt Service Routine for the driver. > > + * @irq: the IRQ on which the interrupt was received. > > + * @dev_id: device-specific info (here a pointer to a net_device struct) > > + * > > + * Returns a value indicating if the interrupt was handled. > > + */ > > +static irqreturn_t et131x_isr(int irq, void *dev_id) > > +{ > > + bool handled = true; > > + bool enable_interrupts = true; > > + struct net_device *netdev = (struct net_device *)dev_id; > > No need to cast a void *. > > [...] > > > +static const struct pci_device_id et131x_pci_table[] = { > > + { PCI_VDEVICE(ATT, ET131X_PCI_DEVICE_ID_GIG), 0UL}, > > + { PCI_VDEVICE(ATT, ET131X_PCI_DEVICE_ID_FAST), 0UL}, > > + {0,} > > Nit: Space after opening curly brace. Ok, I'll change both of these, thanks. Mark
WARNING: multiple messages have this Message-ID (diff)
From: Mark Einon <mark.einon@gmail.com> To: Tobias Klauser <tklauser@distanz.ch> Cc: devel@driverdev.osuosl.org, gregkh@linuxfoundation.org, davem@davemloft.net, linux-kernel@vger.kernel.org, netdev@vger.kernel.org Subject: Re: [RFC PATCH linux-next] et131x: Promote staging et131x driver to drivers/net Date: Tue, 23 Sep 2014 10:46:15 +0100 [thread overview] Message-ID: <20140923094614.GA5130@leicester.auvation.com> (raw) In-Reply-To: <20140923072251.GD4657@distanz.ch> On Tue, Sep 23, 2014 at 09:22:53AM +0200, Tobias Klauser wrote: Hi Tobias, Thanks for the details review. I've replied below - [...] > > +/* et131x_rx_dma_memory_alloc > > + * > > + * Allocates Free buffer ring 1 for sure, free buffer ring 0 if required, > > + * and the Packet Status Ring. > > + */ > > +static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter) > > +{ > > + u8 id; > > + u32 i, j; > > + u32 bufsize; > > + u32 psr_size; > > + u32 fbr_chunksize; > > + struct rx_ring *rx_ring = &adapter->rx_ring; > > + struct fbr_lookup *fbr; > > + > > + /* Alloc memory for the lookup table */ > > + rx_ring->fbr[0] = kmalloc(sizeof(*fbr), GFP_KERNEL); > > + if (rx_ring->fbr[0] == NULL) > > + return -ENOMEM; > > + rx_ring->fbr[1] = kmalloc(sizeof(*fbr), GFP_KERNEL); > > + if (rx_ring->fbr[1] == NULL) > > + return -ENOMEM; > > If you fail here, et131x_rx_dma_memory_free() will be called by > et131x_adapter_memory_free() in the error handling path which in turn > will access the members of the already allocated rx_ring->fbr[0] (e.g. > ->buffsize). Since it is allocated with kmalloc() these members contain > arbitrary values and cause problems in et131x_rx_dma_memory_free(). I' > suggest to do the cleanup (i.e. free rx_ring->fbr[0] directly here and > not call et131x_rx_dma_memory_free() in the error handling path. You > might want to check that memory allocation failures are properly handled > in the rest of the driver as well. There are multiple other cases where > et131x_adapter_memory_free() is called on partially > allocated/initialized memory. > I don't think this is the case - the members of rx_ring->fbr[x] are not accessed unless this pointer is non-NULL in et131x_rx_dma_memory_free() (see code snippet below), which is exactly why the kmalloc code above would fail, so buffsize never gets accessed and the code is cleaned up correctly. Actually, for further explanation, this thread discusses these changes: http://www.spinics.net/lists/linux-driver-devel/msg42128.html > > +/* et131x_rx_dma_memory_free - Free all memory allocated within this module */ > > +static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter) > > +{ [...] > > + /* Free Free Buffer Rings */ > > + for (id = 0; id < NUM_FBRS; id++) { > > + fbr = rx_ring->fbr[id]; > > + > > + if (!fbr || !fbr->ring_virtaddr) > > + continue; > > + > > + /* First the packet memory */ > > + for (ii = 0; ii < fbr->num_entries / FBR_CHUNKS; ii++) { > > + if (fbr->mem_virtaddrs[ii]) { > > + bufsize = fbr->buffsize * FBR_CHUNKS; [...] > > +static SIMPLE_DEV_PM_OPS(et131x_pm_ops, et131x_suspend, et131x_resume); > > +#define ET131X_PM_OPS (&et131x_pm_ops) > > +#else > > +#define ET131X_PM_OPS NULL > > +#endif > > No need for the #define here, just assigne et131x_pm_ops to .driver.pm > directly, its members will be NULL and thus never called. Also, you can > make et131x_pm_ops const. Ok, I can change this. Btw, this appears to be a fairly standard way of using .driver.pm among ethernet drivers, e.g. see 3com/3c59x.c, atheros, marvell... - perhaps there is a case for changing all instances of this code? > > + > > +/* et131x_isr - The Interrupt Service Routine for the driver. > > + * @irq: the IRQ on which the interrupt was received. > > + * @dev_id: device-specific info (here a pointer to a net_device struct) > > + * > > + * Returns a value indicating if the interrupt was handled. > > + */ > > +static irqreturn_t et131x_isr(int irq, void *dev_id) > > +{ > > + bool handled = true; > > + bool enable_interrupts = true; > > + struct net_device *netdev = (struct net_device *)dev_id; > > No need to cast a void *. > > [...] > > > +static const struct pci_device_id et131x_pci_table[] = { > > + { PCI_VDEVICE(ATT, ET131X_PCI_DEVICE_ID_GIG), 0UL}, > > + { PCI_VDEVICE(ATT, ET131X_PCI_DEVICE_ID_FAST), 0UL}, > > + {0,} > > Nit: Space after opening curly brace. Ok, I'll change both of these, thanks. Mark
next prev parent reply other threads:[~2014-09-23 9:47 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-09-22 21:28 [RFC PATCH linux-next] et131x: Promote staging et131x driver to drivers/net Mark Einon 2014-09-22 23:56 ` Angus Gibson 2014-09-23 9:51 ` Mark Einon 2014-09-23 9:51 ` Mark Einon 2014-09-23 1:57 ` Joe Perches 2014-09-23 9:50 ` Mark Einon 2014-09-23 9:50 ` Mark Einon 2014-09-23 7:22 ` Tobias Klauser 2014-09-23 7:22 ` Tobias Klauser 2014-09-23 9:46 ` Mark Einon [this message] 2014-09-23 9:46 ` Mark Einon 2014-09-23 10:01 ` Tobias Klauser 2014-09-23 10:01 ` Tobias Klauser 2014-09-23 11:06 ` Mark Einon 2014-09-23 11:06 ` Mark Einon 2014-09-23 19:41 ` [PATCH 1/4] staging: et131x: zero allocation of fbr to prevent random address access Mark Einon 2014-09-23 19:41 ` Mark Einon 2014-09-23 19:41 ` [PATCH 2/4] staging: et131x: don't cast a void* to a struct pointer Mark Einon 2014-09-23 19:41 ` Mark Einon 2014-09-23 19:41 ` [PATCH 3/4] staging: et131x: Add space after { in pci ID table Mark Einon 2014-09-23 19:41 ` Mark Einon 2014-09-23 19:41 ` [PATCH 4/4] staging: et131x: Remove unnecessary defines to enable driver PM Mark Einon 2014-09-23 19:41 ` Mark Einon 2014-09-23 19:02 ` [RFC PATCH linux-next] et131x: Promote staging et131x driver to drivers/net Francois Romieu 2014-09-23 19:17 ` Joe Perches 2014-09-23 20:07 ` Francois Romieu 2014-09-23 20:07 ` Francois Romieu 2014-09-23 20:05 ` [PATCH v2] " Mark Einon 2014-09-23 20:05 ` Mark Einon 2014-09-23 21:07 ` Joe Perches 2014-09-23 21:07 ` Joe Perches 2014-09-24 8:54 ` [PATCH 1/4] staging: et131x: Use ether_addr_copy when copying ethernet addresses Mark Einon 2014-09-24 8:54 ` Mark Einon 2014-09-24 8:54 ` [PATCH 2/4] staging: et131x: Cat some lines less than 80 columns Mark Einon 2014-09-24 8:54 ` Mark Einon 2014-09-24 8:54 ` [PATCH 3/4] staging: et131x: Remove unnecessary OOM message Mark Einon 2014-09-24 8:54 ` Mark Einon 2014-09-24 8:54 ` [PATCH 4/4] staging: et131x: Remove unnecessary parentheses Mark Einon 2014-09-24 8:54 ` Mark Einon 2014-09-24 9:00 ` [PATCH v2] et131x: Promote staging et131x driver to drivers/net Mark Einon 2014-09-24 9:00 ` Mark Einon -- strict thread matches above, loose matches on Subject: below -- 2013-01-18 20:40 [RFC PATCH linux-next] " Mark Einon 2013-01-18 22:57 ` Greg KH 2013-01-19 11:03 ` Dan Carpenter 2013-01-21 23:44 ` Mark Einon 2013-01-22 6:20 ` Dan Carpenter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20140923094614.GA5130@leicester.auvation.com \ --to=mark.einon@gmail.com \ --cc=davem@davemloft.net \ --cc=devel@driverdev.osuosl.org \ --cc=gregkh@linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=tklauser@distanz.ch \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.