From: Tobias Klauser <tklauser@distanz.ch> To: Mark Einon <mark.einon@gmail.com> 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 12:01:07 +0200 [thread overview] Message-ID: <20140923100106.GE4657@distanz.ch> (raw) In-Reply-To: <20140923094614.GA5130@leicester.auvation.com> On 2014-09-23 at 11:46:15 +0200, Mark Einon <mark.einon@gmail.com> wrote: > 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 Hm, won't rx_ring->fbr[0] be accessed, since it was successfully allocated? However, it isn't properly initialized, so at least the ->ring_virtaddr will get accessed and if this is non-NULL by accident also further members? > > > +/* 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? Yes, this would probably make sense. Looks like a good job for a coccinelle patch.
WARNING: multiple messages have this Message-ID (diff)
From: Tobias Klauser <tklauser@distanz.ch> To: Mark Einon <mark.einon@gmail.com> 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 12:01:07 +0200 [thread overview] Message-ID: <20140923100106.GE4657@distanz.ch> (raw) In-Reply-To: <20140923094614.GA5130@leicester.auvation.com> On 2014-09-23 at 11:46:15 +0200, Mark Einon <mark.einon@gmail.com> wrote: > 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 Hm, won't rx_ring->fbr[0] be accessed, since it was successfully allocated? However, it isn't properly initialized, so at least the ->ring_virtaddr will get accessed and if this is non-NULL by accident also further members? > > > +/* 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? Yes, this would probably make sense. Looks like a good job for a coccinelle patch.
next prev parent reply other threads:[~2014-09-23 10:01 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 2014-09-23 9:46 ` Mark Einon 2014-09-23 10:01 ` Tobias Klauser [this message] 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=20140923100106.GE4657@distanz.ch \ --to=tklauser@distanz.ch \ --cc=davem@davemloft.net \ --cc=devel@driverdev.osuosl.org \ --cc=gregkh@linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.einon@gmail.com \ --cc=netdev@vger.kernel.org \ /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.