* [PATCH v2] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-06 11:14 ` Vadim Lomovtsev
0 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 11:14 UTC (permalink / raw)
To: linux-arm-kernel
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
It is too expensive to pass u64 values via linked list, instead
allocate array for them by overall number of mac addresses from netdev.
This eventually removes multiple kmalloc() calls, aviod memory
fragmentation and allow to put single null check on kmalloc
return value in order to prevent a potential null pointer dereference.
Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
Changes from v1 to v2:
- C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
---
drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
2 files changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
index 5fc46c5a4f36..448d1fafc827 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -265,14 +265,9 @@ struct nicvf_drv_stats {
struct cavium_ptp;
-struct xcast_addr {
- struct list_head list;
- u64 addr;
-};
-
struct xcast_addr_list {
- struct list_head list;
int count;
+ u64 mc[];
};
struct nicvf_work {
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31fef729..a26d8bc92e01 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
work.work);
struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
union nic_mbx mbx = {};
- struct xcast_addr *xaddr, *next;
+ u8 idx = 0;
if (!vf_work)
return;
@@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
/* check if we have any specific MACs to be added to PF DMAC filter */
if (vf_work->mc) {
/* now go through kernel list of MACs and add them one by one */
- list_for_each_entry_safe(xaddr, next,
- &vf_work->mc->list, list) {
+ for (idx = 0; idx < vf_work->mc->count; idx++) {
mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
- mbx.xcast.data.mac = xaddr->addr;
+ mbx.xcast.data.mac = vf_work->mc->mc[idx];
nicvf_send_msg_to_pf(nic, &mbx);
-
- /* after receiving ACK from PF release memory */
- list_del(&xaddr->list);
- kfree(xaddr);
- vf_work->mc->count--;
}
kfree(vf_work->mc);
}
@@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
mode |= BGX_XCAST_MCAST_FILTER;
/* here we need to copy mc addrs */
if (netdev_mc_count(netdev)) {
- struct xcast_addr *xaddr;
-
- mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
- INIT_LIST_HEAD(&mc_list->list);
+ mc_list = kmalloc(sizeof(*mc_list) +
+ sizeof(u64) * netdev_mc_count(netdev),
+ GFP_ATOMIC);
+ if (unlikely(!mc_list))
+ return;
+ mc_list->count = 0;
netdev_hw_addr_list_for_each(ha, &netdev->mc) {
- xaddr = kmalloc(sizeof(*xaddr),
- GFP_ATOMIC);
- xaddr->addr =
+ mc_list->mc[mc_list->count] =
ether_addr_to_u64(ha->addr);
- list_add_tail(&xaddr->list,
- &mc_list->list);
mc_list->count++;
}
}
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v2] net: thunderx: rework mac addresses list to u64 array
2018-04-06 11:14 ` Vadim Lomovtsev
@ 2018-04-06 11:36 ` Gustavo A. R. Silva
-1 siblings, 0 replies; 42+ messages in thread
From: Gustavo A. R. Silva @ 2018-04-06 11:36 UTC (permalink / raw)
To: Vadim Lomovtsev, sgoutham, sunil.kovvuri, rric, linux-arm-kernel,
netdev, linux-kernel, davem
Cc: dnelson, Vadim Lomovtsev
Hi Vadim,
On 04/06/2018 06:14 AM, Vadim Lomovtsev wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>
> It is too expensive to pass u64 values via linked list, instead
> allocate array for them by overall number of mac addresses from netdev.
>
> This eventually removes multiple kmalloc() calls, aviod memory
> fragmentation and allow to put single null check on kmalloc
> return value in order to prevent a potential null pointer dereference.
>
> Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
It'd be nice if you add:
Reported-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Thanks
--
Gustavo
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> ---
> Changes from v1 to v2:
> - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
>
> ---
> drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
> drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
> 2 files changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> index 5fc46c5a4f36..448d1fafc827 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
>
> struct cavium_ptp;
>
> -struct xcast_addr {
> - struct list_head list;
> - u64 addr;
> -};
> -
> struct xcast_addr_list {
> - struct list_head list;
> int count;
> + u64 mc[];
> };
>
> struct nicvf_work {
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index 1e9a31fef729..a26d8bc92e01 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> work.work);
> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + u8 idx = 0;
>
> if (!vf_work)
> return;
> @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> /* check if we have any specific MACs to be added to PF DMAC filter */
> if (vf_work->mc) {
> /* now go through kernel list of MACs and add them one by one */
> - list_for_each_entry_safe(xaddr, next,
> - &vf_work->mc->list, list) {
> + for (idx = 0; idx < vf_work->mc->count; idx++) {
> mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
> - mbx.xcast.data.mac = xaddr->addr;
> + mbx.xcast.data.mac = vf_work->mc->mc[idx];
> nicvf_send_msg_to_pf(nic, &mbx);
> -
> - /* after receiving ACK from PF release memory */
> - list_del(&xaddr->list);
> - kfree(xaddr);
> - vf_work->mc->count--;
> }
> kfree(vf_work->mc);
> }
> @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
> mode |= BGX_XCAST_MCAST_FILTER;
> /* here we need to copy mc addrs */
> if (netdev_mc_count(netdev)) {
> - struct xcast_addr *xaddr;
> -
> - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
> - INIT_LIST_HEAD(&mc_list->list);
> + mc_list = kmalloc(sizeof(*mc_list) +
> + sizeof(u64) * netdev_mc_count(netdev),
> + GFP_ATOMIC);
> + if (unlikely(!mc_list))
> + return;
> + mc_list->count = 0;
> netdev_hw_addr_list_for_each(ha, &netdev->mc) {
> - xaddr = kmalloc(sizeof(*xaddr),
> - GFP_ATOMIC);
> - xaddr->addr =
> + mc_list->mc[mc_list->count] =
> ether_addr_to_u64(ha->addr);
> - list_add_tail(&xaddr->list,
> - &mc_list->list);
> mc_list->count++;
> }
> }
>
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v2] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-06 11:36 ` Gustavo A. R. Silva
0 siblings, 0 replies; 42+ messages in thread
From: Gustavo A. R. Silva @ 2018-04-06 11:36 UTC (permalink / raw)
To: linux-arm-kernel
Hi Vadim,
On 04/06/2018 06:14 AM, Vadim Lomovtsev wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>
> It is too expensive to pass u64 values via linked list, instead
> allocate array for them by overall number of mac addresses from netdev.
>
> This eventually removes multiple kmalloc() calls, aviod memory
> fragmentation and allow to put single null check on kmalloc
> return value in order to prevent a potential null pointer dereference.
>
> Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
It'd be nice if you add:
Reported-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Thanks
--
Gustavo
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> ---
> Changes from v1 to v2:
> - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
>
> ---
> drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
> drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
> 2 files changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> index 5fc46c5a4f36..448d1fafc827 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
>
> struct cavium_ptp;
>
> -struct xcast_addr {
> - struct list_head list;
> - u64 addr;
> -};
> -
> struct xcast_addr_list {
> - struct list_head list;
> int count;
> + u64 mc[];
> };
>
> struct nicvf_work {
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index 1e9a31fef729..a26d8bc92e01 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> work.work);
> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + u8 idx = 0;
>
> if (!vf_work)
> return;
> @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> /* check if we have any specific MACs to be added to PF DMAC filter */
> if (vf_work->mc) {
> /* now go through kernel list of MACs and add them one by one */
> - list_for_each_entry_safe(xaddr, next,
> - &vf_work->mc->list, list) {
> + for (idx = 0; idx < vf_work->mc->count; idx++) {
> mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
> - mbx.xcast.data.mac = xaddr->addr;
> + mbx.xcast.data.mac = vf_work->mc->mc[idx];
> nicvf_send_msg_to_pf(nic, &mbx);
> -
> - /* after receiving ACK from PF release memory */
> - list_del(&xaddr->list);
> - kfree(xaddr);
> - vf_work->mc->count--;
> }
> kfree(vf_work->mc);
> }
> @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
> mode |= BGX_XCAST_MCAST_FILTER;
> /* here we need to copy mc addrs */
> if (netdev_mc_count(netdev)) {
> - struct xcast_addr *xaddr;
> -
> - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
> - INIT_LIST_HEAD(&mc_list->list);
> + mc_list = kmalloc(sizeof(*mc_list) +
> + sizeof(u64) * netdev_mc_count(netdev),
> + GFP_ATOMIC);
> + if (unlikely(!mc_list))
> + return;
> + mc_list->count = 0;
> netdev_hw_addr_list_for_each(ha, &netdev->mc) {
> - xaddr = kmalloc(sizeof(*xaddr),
> - GFP_ATOMIC);
> - xaddr->addr =
> + mc_list->mc[mc_list->count] =
> ether_addr_to_u64(ha->addr);
> - list_add_tail(&xaddr->list,
> - &mc_list->list);
> mc_list->count++;
> }
> }
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2] net: thunderx: rework mac addresses list to u64 array
2018-04-06 11:36 ` Gustavo A. R. Silva
@ 2018-04-06 11:43 ` Vadim Lomovtsev
-1 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 11:43 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, davem, dnelson, Vadim Lomovtsev
Hi Gustavo,
On Fri, Apr 06, 2018 at 06:36:28AM -0500, Gustavo A. R. Silva wrote:
> Hi Vadim,
>
> On 04/06/2018 06:14 AM, Vadim Lomovtsev wrote:
> > From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> >
> > It is too expensive to pass u64 values via linked list, instead
> > allocate array for them by overall number of mac addresses from netdev.
> >
> > This eventually removes multiple kmalloc() calls, aviod memory
> > fragmentation and allow to put single null check on kmalloc
> > return value in order to prevent a potential null pointer dereference.
> >
> > Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> > Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
>
> It'd be nice if you add:
>
> Reported-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Ok, I could do that.
Just to explain .. I didn't do it yet since I get such report from
Dan Carpenter intially (https://www.spinics.net/lists/linux-kernel-janitors/msg40720.html)
and was working on it when found you patches, so then asking you to give
me some time to prepare and test update to driver.
So would it be acceptable put two tags 'Reported-by:' then ?
WBR,
Vadim
>
> Thanks
> --
> Gustavo
>
> > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> > ---
> > Changes from v1 to v2:
> > - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
> >
> > ---
> > drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
> > drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
> > 2 files changed, 11 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> > index 5fc46c5a4f36..448d1fafc827 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nic.h
> > +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> > @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
> > struct cavium_ptp;
> > -struct xcast_addr {
> > - struct list_head list;
> > - u64 addr;
> > -};
> > -
> > struct xcast_addr_list {
> > - struct list_head list;
> > int count;
> > + u64 mc[];
> > };
> > struct nicvf_work {
> > diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > index 1e9a31fef729..a26d8bc92e01 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > work.work);
> > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > union nic_mbx mbx = {};
> > - struct xcast_addr *xaddr, *next;
> > + u8 idx = 0;
> > if (!vf_work)
> > return;
> > @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > /* check if we have any specific MACs to be added to PF DMAC filter */
> > if (vf_work->mc) {
> > /* now go through kernel list of MACs and add them one by one */
> > - list_for_each_entry_safe(xaddr, next,
> > - &vf_work->mc->list, list) {
> > + for (idx = 0; idx < vf_work->mc->count; idx++) {
> > mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
> > - mbx.xcast.data.mac = xaddr->addr;
> > + mbx.xcast.data.mac = vf_work->mc->mc[idx];
> > nicvf_send_msg_to_pf(nic, &mbx);
> > -
> > - /* after receiving ACK from PF release memory */
> > - list_del(&xaddr->list);
> > - kfree(xaddr);
> > - vf_work->mc->count--;
> > }
> > kfree(vf_work->mc);
> > }
> > @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
> > mode |= BGX_XCAST_MCAST_FILTER;
> > /* here we need to copy mc addrs */
> > if (netdev_mc_count(netdev)) {
> > - struct xcast_addr *xaddr;
> > -
> > - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
> > - INIT_LIST_HEAD(&mc_list->list);
> > + mc_list = kmalloc(sizeof(*mc_list) +
> > + sizeof(u64) * netdev_mc_count(netdev),
> > + GFP_ATOMIC);
> > + if (unlikely(!mc_list))
> > + return;
> > + mc_list->count = 0;
> > netdev_hw_addr_list_for_each(ha, &netdev->mc) {
> > - xaddr = kmalloc(sizeof(*xaddr),
> > - GFP_ATOMIC);
> > - xaddr->addr =
> > + mc_list->mc[mc_list->count] =
> > ether_addr_to_u64(ha->addr);
> > - list_add_tail(&xaddr->list,
> > - &mc_list->list);
> > mc_list->count++;
> > }
> > }
> >
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v2] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-06 11:43 ` Vadim Lomovtsev
0 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 11:43 UTC (permalink / raw)
To: linux-arm-kernel
Hi Gustavo,
On Fri, Apr 06, 2018 at 06:36:28AM -0500, Gustavo A. R. Silva wrote:
> Hi Vadim,
>
> On 04/06/2018 06:14 AM, Vadim Lomovtsev wrote:
> > From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> >
> > It is too expensive to pass u64 values via linked list, instead
> > allocate array for them by overall number of mac addresses from netdev.
> >
> > This eventually removes multiple kmalloc() calls, aviod memory
> > fragmentation and allow to put single null check on kmalloc
> > return value in order to prevent a potential null pointer dereference.
> >
> > Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> > Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
>
> It'd be nice if you add:
>
> Reported-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Ok, I could do that.
Just to explain .. I didn't do it yet since I get such report from
Dan Carpenter intially (https://www.spinics.net/lists/linux-kernel-janitors/msg40720.html)
and was working on it when found you patches, so then asking you to give
me some time to prepare and test update to driver.
So would it be acceptable put two tags 'Reported-by:' then ?
WBR,
Vadim
>
> Thanks
> --
> Gustavo
>
> > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> > ---
> > Changes from v1 to v2:
> > - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
> >
> > ---
> > drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
> > drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
> > 2 files changed, 11 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> > index 5fc46c5a4f36..448d1fafc827 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nic.h
> > +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> > @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
> > struct cavium_ptp;
> > -struct xcast_addr {
> > - struct list_head list;
> > - u64 addr;
> > -};
> > -
> > struct xcast_addr_list {
> > - struct list_head list;
> > int count;
> > + u64 mc[];
> > };
> > struct nicvf_work {
> > diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > index 1e9a31fef729..a26d8bc92e01 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > work.work);
> > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > union nic_mbx mbx = {};
> > - struct xcast_addr *xaddr, *next;
> > + u8 idx = 0;
> > if (!vf_work)
> > return;
> > @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > /* check if we have any specific MACs to be added to PF DMAC filter */
> > if (vf_work->mc) {
> > /* now go through kernel list of MACs and add them one by one */
> > - list_for_each_entry_safe(xaddr, next,
> > - &vf_work->mc->list, list) {
> > + for (idx = 0; idx < vf_work->mc->count; idx++) {
> > mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
> > - mbx.xcast.data.mac = xaddr->addr;
> > + mbx.xcast.data.mac = vf_work->mc->mc[idx];
> > nicvf_send_msg_to_pf(nic, &mbx);
> > -
> > - /* after receiving ACK from PF release memory */
> > - list_del(&xaddr->list);
> > - kfree(xaddr);
> > - vf_work->mc->count--;
> > }
> > kfree(vf_work->mc);
> > }
> > @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
> > mode |= BGX_XCAST_MCAST_FILTER;
> > /* here we need to copy mc addrs */
> > if (netdev_mc_count(netdev)) {
> > - struct xcast_addr *xaddr;
> > -
> > - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
> > - INIT_LIST_HEAD(&mc_list->list);
> > + mc_list = kmalloc(sizeof(*mc_list) +
> > + sizeof(u64) * netdev_mc_count(netdev),
> > + GFP_ATOMIC);
> > + if (unlikely(!mc_list))
> > + return;
> > + mc_list->count = 0;
> > netdev_hw_addr_list_for_each(ha, &netdev->mc) {
> > - xaddr = kmalloc(sizeof(*xaddr),
> > - GFP_ATOMIC);
> > - xaddr->addr =
> > + mc_list->mc[mc_list->count] =
> > ether_addr_to_u64(ha->addr);
> > - list_add_tail(&xaddr->list,
> > - &mc_list->list);
> > mc_list->count++;
> > }
> > }
> >
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2] net: thunderx: rework mac addresses list to u64 array
2018-04-06 11:43 ` Vadim Lomovtsev
@ 2018-04-06 11:47 ` Gustavo A. R. Silva
-1 siblings, 0 replies; 42+ messages in thread
From: Gustavo A. R. Silva @ 2018-04-06 11:47 UTC (permalink / raw)
To: Vadim Lomovtsev
Cc: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, davem, dnelson, Vadim Lomovtsev
On 04/06/2018 06:43 AM, Vadim Lomovtsev wrote:
> Hi Gustavo,
>
> On Fri, Apr 06, 2018 at 06:36:28AM -0500, Gustavo A. R. Silva wrote:
>> Hi Vadim,
>>
>> On 04/06/2018 06:14 AM, Vadim Lomovtsev wrote:
>>> From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>>>
>>> It is too expensive to pass u64 values via linked list, instead
>>> allocate array for them by overall number of mac addresses from netdev.
>>>
>>> This eventually removes multiple kmalloc() calls, aviod memory
>>> fragmentation and allow to put single null check on kmalloc
>>> return value in order to prevent a potential null pointer dereference.
>>>
>>> Addresses-Coverity-ID: 1467429 ("Dereference null return value")
>>> Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
>>
>> It'd be nice if you add:
>>
>> Reported-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>
> Ok, I could do that.
>
> Just to explain .. I didn't do it yet since I get such report from
> Dan Carpenter intially (https://www.spinics.net/lists/linux-kernel-janitors/msg40720.html)
> and was working on it when found you patches, so then asking you to give
> me some time to prepare and test update to driver.
>
Oh I got it. Not big deal. I think in this case you should add Dan's
Reported-by instead.
BTW nice patch. :)
Thanks
--
Gustavo
> So would it be acceptable put two tags 'Reported-by:' then ?
>
> WBR,
> Vadim
>
>>
>> Thanks
>> --
>> Gustavo
>>
>>> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>>> ---
>>> Changes from v1 to v2:
>>> - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
>>>
>>> ---
>>> drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
>>> drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
>>> 2 files changed, 11 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
>>> index 5fc46c5a4f36..448d1fafc827 100644
>>> --- a/drivers/net/ethernet/cavium/thunder/nic.h
>>> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
>>> @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
>>> struct cavium_ptp;
>>> -struct xcast_addr {
>>> - struct list_head list;
>>> - u64 addr;
>>> -};
>>> -
>>> struct xcast_addr_list {
>>> - struct list_head list;
>>> int count;
>>> + u64 mc[];
>>> };
>>> struct nicvf_work {
>>> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
>>> index 1e9a31fef729..a26d8bc92e01 100644
>>> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
>>> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
>>> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
>>> work.work);
>>> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
>>> union nic_mbx mbx = {};
>>> - struct xcast_addr *xaddr, *next;
>>> + u8 idx = 0;
>>> if (!vf_work)
>>> return;
>>> @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
>>> /* check if we have any specific MACs to be added to PF DMAC filter */
>>> if (vf_work->mc) {
>>> /* now go through kernel list of MACs and add them one by one */
>>> - list_for_each_entry_safe(xaddr, next,
>>> - &vf_work->mc->list, list) {
>>> + for (idx = 0; idx < vf_work->mc->count; idx++) {
>>> mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
>>> - mbx.xcast.data.mac = xaddr->addr;
>>> + mbx.xcast.data.mac = vf_work->mc->mc[idx];
>>> nicvf_send_msg_to_pf(nic, &mbx);
>>> -
>>> - /* after receiving ACK from PF release memory */
>>> - list_del(&xaddr->list);
>>> - kfree(xaddr);
>>> - vf_work->mc->count--;
>>> }
>>> kfree(vf_work->mc);
>>> }
>>> @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
>>> mode |= BGX_XCAST_MCAST_FILTER;
>>> /* here we need to copy mc addrs */
>>> if (netdev_mc_count(netdev)) {
>>> - struct xcast_addr *xaddr;
>>> -
>>> - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
>>> - INIT_LIST_HEAD(&mc_list->list);
>>> + mc_list = kmalloc(sizeof(*mc_list) +
>>> + sizeof(u64) * netdev_mc_count(netdev),
>>> + GFP_ATOMIC);
>>> + if (unlikely(!mc_list))
>>> + return;
>>> + mc_list->count = 0;
>>> netdev_hw_addr_list_for_each(ha, &netdev->mc) {
>>> - xaddr = kmalloc(sizeof(*xaddr),
>>> - GFP_ATOMIC);
>>> - xaddr->addr =
>>> + mc_list->mc[mc_list->count] =
>>> ether_addr_to_u64(ha->addr);
>>> - list_add_tail(&xaddr->list,
>>> - &mc_list->list);
>>> mc_list->count++;
>>> }
>>> }
>>>
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v2] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-06 11:47 ` Gustavo A. R. Silva
0 siblings, 0 replies; 42+ messages in thread
From: Gustavo A. R. Silva @ 2018-04-06 11:47 UTC (permalink / raw)
To: linux-arm-kernel
On 04/06/2018 06:43 AM, Vadim Lomovtsev wrote:
> Hi Gustavo,
>
> On Fri, Apr 06, 2018 at 06:36:28AM -0500, Gustavo A. R. Silva wrote:
>> Hi Vadim,
>>
>> On 04/06/2018 06:14 AM, Vadim Lomovtsev wrote:
>>> From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>>>
>>> It is too expensive to pass u64 values via linked list, instead
>>> allocate array for them by overall number of mac addresses from netdev.
>>>
>>> This eventually removes multiple kmalloc() calls, aviod memory
>>> fragmentation and allow to put single null check on kmalloc
>>> return value in order to prevent a potential null pointer dereference.
>>>
>>> Addresses-Coverity-ID: 1467429 ("Dereference null return value")
>>> Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
>>
>> It'd be nice if you add:
>>
>> Reported-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>
> Ok, I could do that.
>
> Just to explain .. I didn't do it yet since I get such report from
> Dan Carpenter intially (https://www.spinics.net/lists/linux-kernel-janitors/msg40720.html)
> and was working on it when found you patches, so then asking you to give
> me some time to prepare and test update to driver.
>
Oh I got it. Not big deal. I think in this case you should add Dan's
Reported-by instead.
BTW nice patch. :)
Thanks
--
Gustavo
> So would it be acceptable put two tags 'Reported-by:' then ?
>
> WBR,
> Vadim
>
>>
>> Thanks
>> --
>> Gustavo
>>
>>> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>>> ---
>>> Changes from v1 to v2:
>>> - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
>>>
>>> ---
>>> drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
>>> drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
>>> 2 files changed, 11 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
>>> index 5fc46c5a4f36..448d1fafc827 100644
>>> --- a/drivers/net/ethernet/cavium/thunder/nic.h
>>> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
>>> @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
>>> struct cavium_ptp;
>>> -struct xcast_addr {
>>> - struct list_head list;
>>> - u64 addr;
>>> -};
>>> -
>>> struct xcast_addr_list {
>>> - struct list_head list;
>>> int count;
>>> + u64 mc[];
>>> };
>>> struct nicvf_work {
>>> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
>>> index 1e9a31fef729..a26d8bc92e01 100644
>>> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
>>> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
>>> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
>>> work.work);
>>> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
>>> union nic_mbx mbx = {};
>>> - struct xcast_addr *xaddr, *next;
>>> + u8 idx = 0;
>>> if (!vf_work)
>>> return;
>>> @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
>>> /* check if we have any specific MACs to be added to PF DMAC filter */
>>> if (vf_work->mc) {
>>> /* now go through kernel list of MACs and add them one by one */
>>> - list_for_each_entry_safe(xaddr, next,
>>> - &vf_work->mc->list, list) {
>>> + for (idx = 0; idx < vf_work->mc->count; idx++) {
>>> mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
>>> - mbx.xcast.data.mac = xaddr->addr;
>>> + mbx.xcast.data.mac = vf_work->mc->mc[idx];
>>> nicvf_send_msg_to_pf(nic, &mbx);
>>> -
>>> - /* after receiving ACK from PF release memory */
>>> - list_del(&xaddr->list);
>>> - kfree(xaddr);
>>> - vf_work->mc->count--;
>>> }
>>> kfree(vf_work->mc);
>>> }
>>> @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
>>> mode |= BGX_XCAST_MCAST_FILTER;
>>> /* here we need to copy mc addrs */
>>> if (netdev_mc_count(netdev)) {
>>> - struct xcast_addr *xaddr;
>>> -
>>> - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
>>> - INIT_LIST_HEAD(&mc_list->list);
>>> + mc_list = kmalloc(sizeof(*mc_list) +
>>> + sizeof(u64) * netdev_mc_count(netdev),
>>> + GFP_ATOMIC);
>>> + if (unlikely(!mc_list))
>>> + return;
>>> + mc_list->count = 0;
>>> netdev_hw_addr_list_for_each(ha, &netdev->mc) {
>>> - xaddr = kmalloc(sizeof(*xaddr),
>>> - GFP_ATOMIC);
>>> - xaddr->addr =
>>> + mc_list->mc[mc_list->count] =
>>> ether_addr_to_u64(ha->addr);
>>> - list_add_tail(&xaddr->list,
>>> - &mc_list->list);
>>> mc_list->count++;
>>> }
>>> }
>>>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2] net: thunderx: rework mac addresses list to u64 array
2018-04-06 11:47 ` Gustavo A. R. Silva
@ 2018-04-06 11:53 ` Vadim Lomovtsev
-1 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 11:53 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, davem, dnelson, Vadim Lomovtsev
On Fri, Apr 06, 2018 at 06:47:26AM -0500, Gustavo A. R. Silva wrote:
>
>
> On 04/06/2018 06:43 AM, Vadim Lomovtsev wrote:
> > Hi Gustavo,
> >
> > On Fri, Apr 06, 2018 at 06:36:28AM -0500, Gustavo A. R. Silva wrote:
> > > Hi Vadim,
> > >
> > > On 04/06/2018 06:14 AM, Vadim Lomovtsev wrote:
> > > > From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> > > >
> > > > It is too expensive to pass u64 values via linked list, instead
> > > > allocate array for them by overall number of mac addresses from netdev.
> > > >
> > > > This eventually removes multiple kmalloc() calls, aviod memory
> > > > fragmentation and allow to put single null check on kmalloc
> > > > return value in order to prevent a potential null pointer dereference.
> > > >
> > > > Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> > > > Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
> > >
> > > It'd be nice if you add:
> > >
> > > Reported-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> >
> > Ok, I could do that.
> >
> > Just to explain .. I didn't do it yet since I get such report from
> > Dan Carpenter intially (https://www.spinics.net/lists/linux-kernel-janitors/msg40720.html)
> > and was working on it when found you patches, so then asking you to give
> > me some time to prepare and test update to driver.
> >
>
> Oh I got it. Not big deal. I think in this case you should add Dan's
> Reported-by instead.
Ok then.
>
> BTW nice patch. :)
>
Thank you for reviewing it.
WBR,
Vadim
> Thanks
> --
> Gustavo
>
> > So would it be acceptable put two tags 'Reported-by:' then ?
> >
> > WBR,
> > Vadim
> >
> > >
> > > Thanks
> > > --
> > > Gustavo
> > >
> > > > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> > > > ---
> > > > Changes from v1 to v2:
> > > > - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
> > > >
> > > > ---
> > > > drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
> > > > drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
> > > > 2 files changed, 11 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> > > > index 5fc46c5a4f36..448d1fafc827 100644
> > > > --- a/drivers/net/ethernet/cavium/thunder/nic.h
> > > > +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> > > > @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
> > > > struct cavium_ptp;
> > > > -struct xcast_addr {
> > > > - struct list_head list;
> > > > - u64 addr;
> > > > -};
> > > > -
> > > > struct xcast_addr_list {
> > > > - struct list_head list;
> > > > int count;
> > > > + u64 mc[];
> > > > };
> > > > struct nicvf_work {
> > > > diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > > > index 1e9a31fef729..a26d8bc92e01 100644
> > > > --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > > > +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > > > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > > > work.work);
> > > > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > > > union nic_mbx mbx = {};
> > > > - struct xcast_addr *xaddr, *next;
> > > > + u8 idx = 0;
> > > > if (!vf_work)
> > > > return;
> > > > @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > > > /* check if we have any specific MACs to be added to PF DMAC filter */
> > > > if (vf_work->mc) {
> > > > /* now go through kernel list of MACs and add them one by one */
> > > > - list_for_each_entry_safe(xaddr, next,
> > > > - &vf_work->mc->list, list) {
> > > > + for (idx = 0; idx < vf_work->mc->count; idx++) {
> > > > mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
> > > > - mbx.xcast.data.mac = xaddr->addr;
> > > > + mbx.xcast.data.mac = vf_work->mc->mc[idx];
> > > > nicvf_send_msg_to_pf(nic, &mbx);
> > > > -
> > > > - /* after receiving ACK from PF release memory */
> > > > - list_del(&xaddr->list);
> > > > - kfree(xaddr);
> > > > - vf_work->mc->count--;
> > > > }
> > > > kfree(vf_work->mc);
> > > > }
> > > > @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
> > > > mode |= BGX_XCAST_MCAST_FILTER;
> > > > /* here we need to copy mc addrs */
> > > > if (netdev_mc_count(netdev)) {
> > > > - struct xcast_addr *xaddr;
> > > > -
> > > > - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
> > > > - INIT_LIST_HEAD(&mc_list->list);
> > > > + mc_list = kmalloc(sizeof(*mc_list) +
> > > > + sizeof(u64) * netdev_mc_count(netdev),
> > > > + GFP_ATOMIC);
> > > > + if (unlikely(!mc_list))
> > > > + return;
> > > > + mc_list->count = 0;
> > > > netdev_hw_addr_list_for_each(ha, &netdev->mc) {
> > > > - xaddr = kmalloc(sizeof(*xaddr),
> > > > - GFP_ATOMIC);
> > > > - xaddr->addr =
> > > > + mc_list->mc[mc_list->count] =
> > > > ether_addr_to_u64(ha->addr);
> > > > - list_add_tail(&xaddr->list,
> > > > - &mc_list->list);
> > > > mc_list->count++;
> > > > }
> > > > }
> > > >
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v2] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-06 11:53 ` Vadim Lomovtsev
0 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 11:53 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 06, 2018 at 06:47:26AM -0500, Gustavo A. R. Silva wrote:
>
>
> On 04/06/2018 06:43 AM, Vadim Lomovtsev wrote:
> > Hi Gustavo,
> >
> > On Fri, Apr 06, 2018 at 06:36:28AM -0500, Gustavo A. R. Silva wrote:
> > > Hi Vadim,
> > >
> > > On 04/06/2018 06:14 AM, Vadim Lomovtsev wrote:
> > > > From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> > > >
> > > > It is too expensive to pass u64 values via linked list, instead
> > > > allocate array for them by overall number of mac addresses from netdev.
> > > >
> > > > This eventually removes multiple kmalloc() calls, aviod memory
> > > > fragmentation and allow to put single null check on kmalloc
> > > > return value in order to prevent a potential null pointer dereference.
> > > >
> > > > Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> > > > Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
> > >
> > > It'd be nice if you add:
> > >
> > > Reported-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> >
> > Ok, I could do that.
> >
> > Just to explain .. I didn't do it yet since I get such report from
> > Dan Carpenter intially (https://www.spinics.net/lists/linux-kernel-janitors/msg40720.html)
> > and was working on it when found you patches, so then asking you to give
> > me some time to prepare and test update to driver.
> >
>
> Oh I got it. Not big deal. I think in this case you should add Dan's
> Reported-by instead.
Ok then.
>
> BTW nice patch. :)
>
Thank you for reviewing it.
WBR,
Vadim
> Thanks
> --
> Gustavo
>
> > So would it be acceptable put two tags 'Reported-by:' then ?
> >
> > WBR,
> > Vadim
> >
> > >
> > > Thanks
> > > --
> > > Gustavo
> > >
> > > > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> > > > ---
> > > > Changes from v1 to v2:
> > > > - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
> > > >
> > > > ---
> > > > drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
> > > > drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
> > > > 2 files changed, 11 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> > > > index 5fc46c5a4f36..448d1fafc827 100644
> > > > --- a/drivers/net/ethernet/cavium/thunder/nic.h
> > > > +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> > > > @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
> > > > struct cavium_ptp;
> > > > -struct xcast_addr {
> > > > - struct list_head list;
> > > > - u64 addr;
> > > > -};
> > > > -
> > > > struct xcast_addr_list {
> > > > - struct list_head list;
> > > > int count;
> > > > + u64 mc[];
> > > > };
> > > > struct nicvf_work {
> > > > diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > > > index 1e9a31fef729..a26d8bc92e01 100644
> > > > --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > > > +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > > > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > > > work.work);
> > > > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > > > union nic_mbx mbx = {};
> > > > - struct xcast_addr *xaddr, *next;
> > > > + u8 idx = 0;
> > > > if (!vf_work)
> > > > return;
> > > > @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > > > /* check if we have any specific MACs to be added to PF DMAC filter */
> > > > if (vf_work->mc) {
> > > > /* now go through kernel list of MACs and add them one by one */
> > > > - list_for_each_entry_safe(xaddr, next,
> > > > - &vf_work->mc->list, list) {
> > > > + for (idx = 0; idx < vf_work->mc->count; idx++) {
> > > > mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
> > > > - mbx.xcast.data.mac = xaddr->addr;
> > > > + mbx.xcast.data.mac = vf_work->mc->mc[idx];
> > > > nicvf_send_msg_to_pf(nic, &mbx);
> > > > -
> > > > - /* after receiving ACK from PF release memory */
> > > > - list_del(&xaddr->list);
> > > > - kfree(xaddr);
> > > > - vf_work->mc->count--;
> > > > }
> > > > kfree(vf_work->mc);
> > > > }
> > > > @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
> > > > mode |= BGX_XCAST_MCAST_FILTER;
> > > > /* here we need to copy mc addrs */
> > > > if (netdev_mc_count(netdev)) {
> > > > - struct xcast_addr *xaddr;
> > > > -
> > > > - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
> > > > - INIT_LIST_HEAD(&mc_list->list);
> > > > + mc_list = kmalloc(sizeof(*mc_list) +
> > > > + sizeof(u64) * netdev_mc_count(netdev),
> > > > + GFP_ATOMIC);
> > > > + if (unlikely(!mc_list))
> > > > + return;
> > > > + mc_list->count = 0;
> > > > netdev_hw_addr_list_for_each(ha, &netdev->mc) {
> > > > - xaddr = kmalloc(sizeof(*xaddr),
> > > > - GFP_ATOMIC);
> > > > - xaddr->addr =
> > > > + mc_list->mc[mc_list->count] =
> > > > ether_addr_to_u64(ha->addr);
> > > > - list_add_tail(&xaddr->list,
> > > > - &mc_list->list);
> > > > mc_list->count++;
> > > > }
> > > > }
> > > >
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] net: thunderx: rework mac addresses list to u64 array
2018-04-06 11:14 ` Vadim Lomovtsev
@ 2018-04-06 11:48 ` Robin Murphy
-1 siblings, 0 replies; 42+ messages in thread
From: Robin Murphy @ 2018-04-06 11:48 UTC (permalink / raw)
To: Vadim Lomovtsev, sgoutham, sunil.kovvuri, rric, linux-arm-kernel,
netdev, linux-kernel, davem
Cc: dnelson, Vadim Lomovtsev, gustavo
On 06/04/18 12:14, Vadim Lomovtsev wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>
> It is too expensive to pass u64 values via linked list, instead
> allocate array for them by overall number of mac addresses from netdev.
>
> This eventually removes multiple kmalloc() calls, aviod memory
> fragmentation and allow to put single null check on kmalloc
> return value in order to prevent a potential null pointer dereference.
>
> Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> ---
> Changes from v1 to v2:
> - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
>
> ---
> drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
> drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
> 2 files changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> index 5fc46c5a4f36..448d1fafc827 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
>
> struct cavium_ptp;
>
> -struct xcast_addr {
> - struct list_head list;
> - u64 addr;
> -};
> -
> struct xcast_addr_list {
> - struct list_head list;
> int count;
> + u64 mc[];
> };
>
> struct nicvf_work {
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index 1e9a31fef729..a26d8bc92e01 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> work.work);
> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + u8 idx = 0;
>
> if (!vf_work)
> return;
> @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> /* check if we have any specific MACs to be added to PF DMAC filter */
> if (vf_work->mc) {
> /* now go through kernel list of MACs and add them one by one */
> - list_for_each_entry_safe(xaddr, next,
> - &vf_work->mc->list, list) {
> + for (idx = 0; idx < vf_work->mc->count; idx++) {
> mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
> - mbx.xcast.data.mac = xaddr->addr;
> + mbx.xcast.data.mac = vf_work->mc->mc[idx];
> nicvf_send_msg_to_pf(nic, &mbx);
> -
> - /* after receiving ACK from PF release memory */
> - list_del(&xaddr->list);
> - kfree(xaddr);
> - vf_work->mc->count--;
> }
> kfree(vf_work->mc);
> }
> @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
> mode |= BGX_XCAST_MCAST_FILTER;
> /* here we need to copy mc addrs */
> if (netdev_mc_count(netdev)) {
> - struct xcast_addr *xaddr;
> -
> - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
> - INIT_LIST_HEAD(&mc_list->list);
> + mc_list = kmalloc(sizeof(*mc_list) +
> + sizeof(u64) * netdev_mc_count(netdev),
FWIW if you really wanted to disambiguate that it's a structure and not
just an array being allocated, then it could be expressed without
explicit arithmetic:
size = offsetof(typeof(*mc_list), mc[netdev_mc_count(netdev)]);
but that's probably just a matter of personal preference at this point.
Robin.
> + GFP_ATOMIC);
> + if (unlikely(!mc_list))
> + return;
> + mc_list->count = 0;
> netdev_hw_addr_list_for_each(ha, &netdev->mc) {
> - xaddr = kmalloc(sizeof(*xaddr),
> - GFP_ATOMIC);
> - xaddr->addr =
> + mc_list->mc[mc_list->count] =
> ether_addr_to_u64(ha->addr);
> - list_add_tail(&xaddr->list,
> - &mc_list->list);
> mc_list->count++;
> }
> }
>
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v2] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-06 11:48 ` Robin Murphy
0 siblings, 0 replies; 42+ messages in thread
From: Robin Murphy @ 2018-04-06 11:48 UTC (permalink / raw)
To: linux-arm-kernel
On 06/04/18 12:14, Vadim Lomovtsev wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>
> It is too expensive to pass u64 values via linked list, instead
> allocate array for them by overall number of mac addresses from netdev.
>
> This eventually removes multiple kmalloc() calls, aviod memory
> fragmentation and allow to put single null check on kmalloc
> return value in order to prevent a potential null pointer dereference.
>
> Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> ---
> Changes from v1 to v2:
> - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
>
> ---
> drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
> drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
> 2 files changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> index 5fc46c5a4f36..448d1fafc827 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
>
> struct cavium_ptp;
>
> -struct xcast_addr {
> - struct list_head list;
> - u64 addr;
> -};
> -
> struct xcast_addr_list {
> - struct list_head list;
> int count;
> + u64 mc[];
> };
>
> struct nicvf_work {
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index 1e9a31fef729..a26d8bc92e01 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> work.work);
> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + u8 idx = 0;
>
> if (!vf_work)
> return;
> @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> /* check if we have any specific MACs to be added to PF DMAC filter */
> if (vf_work->mc) {
> /* now go through kernel list of MACs and add them one by one */
> - list_for_each_entry_safe(xaddr, next,
> - &vf_work->mc->list, list) {
> + for (idx = 0; idx < vf_work->mc->count; idx++) {
> mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
> - mbx.xcast.data.mac = xaddr->addr;
> + mbx.xcast.data.mac = vf_work->mc->mc[idx];
> nicvf_send_msg_to_pf(nic, &mbx);
> -
> - /* after receiving ACK from PF release memory */
> - list_del(&xaddr->list);
> - kfree(xaddr);
> - vf_work->mc->count--;
> }
> kfree(vf_work->mc);
> }
> @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
> mode |= BGX_XCAST_MCAST_FILTER;
> /* here we need to copy mc addrs */
> if (netdev_mc_count(netdev)) {
> - struct xcast_addr *xaddr;
> -
> - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
> - INIT_LIST_HEAD(&mc_list->list);
> + mc_list = kmalloc(sizeof(*mc_list) +
> + sizeof(u64) * netdev_mc_count(netdev),
FWIW if you really wanted to disambiguate that it's a structure and not
just an array being allocated, then it could be expressed without
explicit arithmetic:
size = offsetof(typeof(*mc_list), mc[netdev_mc_count(netdev)]);
but that's probably just a matter of personal preference at this point.
Robin.
> + GFP_ATOMIC);
> + if (unlikely(!mc_list))
> + return;
> + mc_list->count = 0;
> netdev_hw_addr_list_for_each(ha, &netdev->mc) {
> - xaddr = kmalloc(sizeof(*xaddr),
> - GFP_ATOMIC);
> - xaddr->addr =
> + mc_list->mc[mc_list->count] =
> ether_addr_to_u64(ha->addr);
> - list_add_tail(&xaddr->list,
> - &mc_list->list);
> mc_list->count++;
> }
> }
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2] net: thunderx: rework mac addresses list to u64 array
2018-04-06 11:48 ` Robin Murphy
@ 2018-04-06 12:07 ` Vadim Lomovtsev
-1 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 12:07 UTC (permalink / raw)
To: Robin Murphy
Cc: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, davem, dnelson, Vadim Lomovtsev, gustavo
Hi Robin,
On Fri, Apr 06, 2018 at 12:48:40PM +0100, Robin Murphy wrote:
> On 06/04/18 12:14, Vadim Lomovtsev wrote:
> > From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> >
> > It is too expensive to pass u64 values via linked list, instead
> > allocate array for them by overall number of mac addresses from netdev.
> >
> > This eventually removes multiple kmalloc() calls, aviod memory
> > fragmentation and allow to put single null check on kmalloc
> > return value in order to prevent a potential null pointer dereference.
> >
> > Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> > Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
> > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> > ---
> > Changes from v1 to v2:
> > - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
> >
> > ---
> > drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
> > drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
> > 2 files changed, 11 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> > index 5fc46c5a4f36..448d1fafc827 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nic.h
> > +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> > @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
> > struct cavium_ptp;
> > -struct xcast_addr {
> > - struct list_head list;
> > - u64 addr;
> > -};
> > -
> > struct xcast_addr_list {
> > - struct list_head list;
> > int count;
> > + u64 mc[];
> > };
> > struct nicvf_work {
> > diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > index 1e9a31fef729..a26d8bc92e01 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > work.work);
> > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > union nic_mbx mbx = {};
> > - struct xcast_addr *xaddr, *next;
> > + u8 idx = 0;
> > if (!vf_work)
> > return;
> > @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > /* check if we have any specific MACs to be added to PF DMAC filter */
> > if (vf_work->mc) {
> > /* now go through kernel list of MACs and add them one by one */
> > - list_for_each_entry_safe(xaddr, next,
> > - &vf_work->mc->list, list) {
> > + for (idx = 0; idx < vf_work->mc->count; idx++) {
> > mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
> > - mbx.xcast.data.mac = xaddr->addr;
> > + mbx.xcast.data.mac = vf_work->mc->mc[idx];
> > nicvf_send_msg_to_pf(nic, &mbx);
> > -
> > - /* after receiving ACK from PF release memory */
> > - list_del(&xaddr->list);
> > - kfree(xaddr);
> > - vf_work->mc->count--;
> > }
> > kfree(vf_work->mc);
> > }
> > @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
> > mode |= BGX_XCAST_MCAST_FILTER;
> > /* here we need to copy mc addrs */
> > if (netdev_mc_count(netdev)) {
> > - struct xcast_addr *xaddr;
> > -
> > - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
> > - INIT_LIST_HEAD(&mc_list->list);
> > + mc_list = kmalloc(sizeof(*mc_list) +
> > + sizeof(u64) * netdev_mc_count(netdev),
>
> FWIW if you really wanted to disambiguate that it's a structure and not just
> an array being allocated, then it could be expressed without explicit
> arithmetic:
>
> size = offsetof(typeof(*mc_list), mc[netdev_mc_count(netdev)]);
>
> but that's probably just a matter of personal preference at this point.
>
> Robin.
>
Thanks for reviewing it and for hint. From style perspective, I believe,
I should get rid off direct types names also, and use your suggestion here.
I'll update patch to v3, test and re-post.
Thank you for your time.
WBR,
Vadim
> > + GFP_ATOMIC);
> > + if (unlikely(!mc_list))
> > + return;
> > + mc_list->count = 0;
> > netdev_hw_addr_list_for_each(ha, &netdev->mc) {
> > - xaddr = kmalloc(sizeof(*xaddr),
> > - GFP_ATOMIC);
> > - xaddr->addr =
> > + mc_list->mc[mc_list->count] =
> > ether_addr_to_u64(ha->addr);
> > - list_add_tail(&xaddr->list,
> > - &mc_list->list);
> > mc_list->count++;
> > }
> > }
> >
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v2] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-06 12:07 ` Vadim Lomovtsev
0 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 12:07 UTC (permalink / raw)
To: linux-arm-kernel
Hi Robin,
On Fri, Apr 06, 2018 at 12:48:40PM +0100, Robin Murphy wrote:
> On 06/04/18 12:14, Vadim Lomovtsev wrote:
> > From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> >
> > It is too expensive to pass u64 values via linked list, instead
> > allocate array for them by overall number of mac addresses from netdev.
> >
> > This eventually removes multiple kmalloc() calls, aviod memory
> > fragmentation and allow to put single null check on kmalloc
> > return value in order to prevent a potential null pointer dereference.
> >
> > Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> > Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
> > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> > ---
> > Changes from v1 to v2:
> > - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
> >
> > ---
> > drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
> > drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
> > 2 files changed, 11 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> > index 5fc46c5a4f36..448d1fafc827 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nic.h
> > +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> > @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
> > struct cavium_ptp;
> > -struct xcast_addr {
> > - struct list_head list;
> > - u64 addr;
> > -};
> > -
> > struct xcast_addr_list {
> > - struct list_head list;
> > int count;
> > + u64 mc[];
> > };
> > struct nicvf_work {
> > diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > index 1e9a31fef729..a26d8bc92e01 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > work.work);
> > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > union nic_mbx mbx = {};
> > - struct xcast_addr *xaddr, *next;
> > + u8 idx = 0;
> > if (!vf_work)
> > return;
> > @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > /* check if we have any specific MACs to be added to PF DMAC filter */
> > if (vf_work->mc) {
> > /* now go through kernel list of MACs and add them one by one */
> > - list_for_each_entry_safe(xaddr, next,
> > - &vf_work->mc->list, list) {
> > + for (idx = 0; idx < vf_work->mc->count; idx++) {
> > mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
> > - mbx.xcast.data.mac = xaddr->addr;
> > + mbx.xcast.data.mac = vf_work->mc->mc[idx];
> > nicvf_send_msg_to_pf(nic, &mbx);
> > -
> > - /* after receiving ACK from PF release memory */
> > - list_del(&xaddr->list);
> > - kfree(xaddr);
> > - vf_work->mc->count--;
> > }
> > kfree(vf_work->mc);
> > }
> > @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
> > mode |= BGX_XCAST_MCAST_FILTER;
> > /* here we need to copy mc addrs */
> > if (netdev_mc_count(netdev)) {
> > - struct xcast_addr *xaddr;
> > -
> > - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
> > - INIT_LIST_HEAD(&mc_list->list);
> > + mc_list = kmalloc(sizeof(*mc_list) +
> > + sizeof(u64) * netdev_mc_count(netdev),
>
> FWIW if you really wanted to disambiguate that it's a structure and not just
> an array being allocated, then it could be expressed without explicit
> arithmetic:
>
> size = offsetof(typeof(*mc_list), mc[netdev_mc_count(netdev)]);
>
> but that's probably just a matter of personal preference at this point.
>
> Robin.
>
Thanks for reviewing it and for hint. From style perspective, I believe,
I should get rid off direct types names also, and use your suggestion here.
I'll update patch to v3, test and re-post.
Thank you for your time.
WBR,
Vadim
> > + GFP_ATOMIC);
> > + if (unlikely(!mc_list))
> > + return;
> > + mc_list->count = 0;
> > netdev_hw_addr_list_for_each(ha, &netdev->mc) {
> > - xaddr = kmalloc(sizeof(*xaddr),
> > - GFP_ATOMIC);
> > - xaddr->addr =
> > + mc_list->mc[mc_list->count] =
> > ether_addr_to_u64(ha->addr);
> > - list_add_tail(&xaddr->list,
> > - &mc_list->list);
> > mc_list->count++;
> > }
> > }
> >
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3] net: thunderx: rework mac addresses list to u64 array
2018-04-06 11:14 ` Vadim Lomovtsev
@ 2018-04-06 14:04 ` Vadim Lomovtsev
-1 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 14:04 UTC (permalink / raw)
To: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev, linux-kernel
Cc: dnelson, Vadim Lomovtsev
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
It is too expensive to pass u64 values via linked list, instead
allocate array for them by overall number of mac addresses from netdev.
This eventually removes multiple kmalloc() calls, aviod memory
fragmentation and allow to put single null check on kmalloc
return value in order to prevent a potential null pointer dereference.
Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
Changes from v1 to v2:
- C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
Changes from v2 to v3:
- update commit description with 'Reported-by: Dan Carpenter';
- update size calculations for mc list to offsetof() call
instead of explicit arithmetic;
---
drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
2 files changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
index 5fc46c5a4f36..448d1fafc827 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -265,14 +265,9 @@ struct nicvf_drv_stats {
struct cavium_ptp;
-struct xcast_addr {
- struct list_head list;
- u64 addr;
-};
-
struct xcast_addr_list {
- struct list_head list;
int count;
+ u64 mc[];
};
struct nicvf_work {
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31fef729..7d9e58533a83 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
work.work);
struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
union nic_mbx mbx = {};
- struct xcast_addr *xaddr, *next;
+ u8 idx = 0;
if (!vf_work)
return;
@@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
/* check if we have any specific MACs to be added to PF DMAC filter */
if (vf_work->mc) {
/* now go through kernel list of MACs and add them one by one */
- list_for_each_entry_safe(xaddr, next,
- &vf_work->mc->list, list) {
+ for (idx = 0; idx < vf_work->mc->count; idx++) {
mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
- mbx.xcast.data.mac = xaddr->addr;
+ mbx.xcast.data.mac = vf_work->mc->mc[idx];
nicvf_send_msg_to_pf(nic, &mbx);
-
- /* after receiving ACK from PF release memory */
- list_del(&xaddr->list);
- kfree(xaddr);
- vf_work->mc->count--;
}
kfree(vf_work->mc);
}
@@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
mode |= BGX_XCAST_MCAST_FILTER;
/* here we need to copy mc addrs */
if (netdev_mc_count(netdev)) {
- struct xcast_addr *xaddr;
-
- mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
- INIT_LIST_HEAD(&mc_list->list);
+ mc_list = kmalloc(offsetof(typeof(*mc_list),
+ mc[netdev_mc_count(netdev)]),
+ GFP_ATOMIC);
+ if (unlikely(!mc_list))
+ return;
+ mc_list->count = 0;
netdev_hw_addr_list_for_each(ha, &netdev->mc) {
- xaddr = kmalloc(sizeof(*xaddr),
- GFP_ATOMIC);
- xaddr->addr =
+ mc_list->mc[mc_list->count] =
ether_addr_to_u64(ha->addr);
- list_add_tail(&xaddr->list,
- &mc_list->list);
mc_list->count++;
}
}
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v3] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-06 14:04 ` Vadim Lomovtsev
0 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 14:04 UTC (permalink / raw)
To: linux-arm-kernel
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
It is too expensive to pass u64 values via linked list, instead
allocate array for them by overall number of mac addresses from netdev.
This eventually removes multiple kmalloc() calls, aviod memory
fragmentation and allow to put single null check on kmalloc
return value in order to prevent a potential null pointer dereference.
Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
Changes from v1 to v2:
- C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
Changes from v2 to v3:
- update commit description with 'Reported-by: Dan Carpenter';
- update size calculations for mc list to offsetof() call
instead of explicit arithmetic;
---
drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
2 files changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
index 5fc46c5a4f36..448d1fafc827 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -265,14 +265,9 @@ struct nicvf_drv_stats {
struct cavium_ptp;
-struct xcast_addr {
- struct list_head list;
- u64 addr;
-};
-
struct xcast_addr_list {
- struct list_head list;
int count;
+ u64 mc[];
};
struct nicvf_work {
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31fef729..7d9e58533a83 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
work.work);
struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
union nic_mbx mbx = {};
- struct xcast_addr *xaddr, *next;
+ u8 idx = 0;
if (!vf_work)
return;
@@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
/* check if we have any specific MACs to be added to PF DMAC filter */
if (vf_work->mc) {
/* now go through kernel list of MACs and add them one by one */
- list_for_each_entry_safe(xaddr, next,
- &vf_work->mc->list, list) {
+ for (idx = 0; idx < vf_work->mc->count; idx++) {
mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
- mbx.xcast.data.mac = xaddr->addr;
+ mbx.xcast.data.mac = vf_work->mc->mc[idx];
nicvf_send_msg_to_pf(nic, &mbx);
-
- /* after receiving ACK from PF release memory */
- list_del(&xaddr->list);
- kfree(xaddr);
- vf_work->mc->count--;
}
kfree(vf_work->mc);
}
@@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
mode |= BGX_XCAST_MCAST_FILTER;
/* here we need to copy mc addrs */
if (netdev_mc_count(netdev)) {
- struct xcast_addr *xaddr;
-
- mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
- INIT_LIST_HEAD(&mc_list->list);
+ mc_list = kmalloc(offsetof(typeof(*mc_list),
+ mc[netdev_mc_count(netdev)]),
+ GFP_ATOMIC);
+ if (unlikely(!mc_list))
+ return;
+ mc_list->count = 0;
netdev_hw_addr_list_for_each(ha, &netdev->mc) {
- xaddr = kmalloc(sizeof(*xaddr),
- GFP_ATOMIC);
- xaddr->addr =
+ mc_list->mc[mc_list->count] =
ether_addr_to_u64(ha->addr);
- list_add_tail(&xaddr->list,
- &mc_list->list);
mc_list->count++;
}
}
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v3] net: thunderx: rework mac addresses list to u64 array
2018-04-06 14:04 ` Vadim Lomovtsev
@ 2018-04-06 15:16 ` Vadim Lomovtsev
-1 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 15:16 UTC (permalink / raw)
To: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev, linux-kernel
Cc: dnelson, Vadim Lomovtsev
Self-NACK here, because of https://lkml.org/lkml/2018/4/6/724
Sorry for noise.
Vadim
On Fri, Apr 06, 2018 at 07:04:43AM -0700, Vadim Lomovtsev wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>
> It is too expensive to pass u64 values via linked list, instead
> allocate array for them by overall number of mac addresses from netdev.
>
> This eventually removes multiple kmalloc() calls, aviod memory
> fragmentation and allow to put single null check on kmalloc
> return value in order to prevent a potential null pointer dereference.
>
> Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> ---
> Changes from v1 to v2:
> - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
> Changes from v2 to v3:
> - update commit description with 'Reported-by: Dan Carpenter';
> - update size calculations for mc list to offsetof() call
> instead of explicit arithmetic;
> ---
> drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
> drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
> 2 files changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> index 5fc46c5a4f36..448d1fafc827 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
>
> struct cavium_ptp;
>
> -struct xcast_addr {
> - struct list_head list;
> - u64 addr;
> -};
> -
> struct xcast_addr_list {
> - struct list_head list;
> int count;
> + u64 mc[];
> };
>
> struct nicvf_work {
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index 1e9a31fef729..7d9e58533a83 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> work.work);
> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + u8 idx = 0;
>
> if (!vf_work)
> return;
> @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> /* check if we have any specific MACs to be added to PF DMAC filter */
> if (vf_work->mc) {
> /* now go through kernel list of MACs and add them one by one */
> - list_for_each_entry_safe(xaddr, next,
> - &vf_work->mc->list, list) {
> + for (idx = 0; idx < vf_work->mc->count; idx++) {
> mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
> - mbx.xcast.data.mac = xaddr->addr;
> + mbx.xcast.data.mac = vf_work->mc->mc[idx];
> nicvf_send_msg_to_pf(nic, &mbx);
> -
> - /* after receiving ACK from PF release memory */
> - list_del(&xaddr->list);
> - kfree(xaddr);
> - vf_work->mc->count--;
> }
> kfree(vf_work->mc);
> }
> @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
> mode |= BGX_XCAST_MCAST_FILTER;
> /* here we need to copy mc addrs */
> if (netdev_mc_count(netdev)) {
> - struct xcast_addr *xaddr;
> -
> - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
> - INIT_LIST_HEAD(&mc_list->list);
> + mc_list = kmalloc(offsetof(typeof(*mc_list),
> + mc[netdev_mc_count(netdev)]),
> + GFP_ATOMIC);
> + if (unlikely(!mc_list))
> + return;
> + mc_list->count = 0;
> netdev_hw_addr_list_for_each(ha, &netdev->mc) {
> - xaddr = kmalloc(sizeof(*xaddr),
> - GFP_ATOMIC);
> - xaddr->addr =
> + mc_list->mc[mc_list->count] =
> ether_addr_to_u64(ha->addr);
> - list_add_tail(&xaddr->list,
> - &mc_list->list);
> mc_list->count++;
> }
> }
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v3] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-06 15:16 ` Vadim Lomovtsev
0 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 15:16 UTC (permalink / raw)
To: linux-arm-kernel
Self-NACK here, because of https://lkml.org/lkml/2018/4/6/724
Sorry for noise.
Vadim
On Fri, Apr 06, 2018 at 07:04:43AM -0700, Vadim Lomovtsev wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>
> It is too expensive to pass u64 values via linked list, instead
> allocate array for them by overall number of mac addresses from netdev.
>
> This eventually removes multiple kmalloc() calls, aviod memory
> fragmentation and allow to put single null check on kmalloc
> return value in order to prevent a potential null pointer dereference.
>
> Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> ---
> Changes from v1 to v2:
> - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
> Changes from v2 to v3:
> - update commit description with 'Reported-by: Dan Carpenter';
> - update size calculations for mc list to offsetof() call
> instead of explicit arithmetic;
> ---
> drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
> drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
> 2 files changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> index 5fc46c5a4f36..448d1fafc827 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
>
> struct cavium_ptp;
>
> -struct xcast_addr {
> - struct list_head list;
> - u64 addr;
> -};
> -
> struct xcast_addr_list {
> - struct list_head list;
> int count;
> + u64 mc[];
> };
>
> struct nicvf_work {
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index 1e9a31fef729..7d9e58533a83 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> work.work);
> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + u8 idx = 0;
>
> if (!vf_work)
> return;
> @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> /* check if we have any specific MACs to be added to PF DMAC filter */
> if (vf_work->mc) {
> /* now go through kernel list of MACs and add them one by one */
> - list_for_each_entry_safe(xaddr, next,
> - &vf_work->mc->list, list) {
> + for (idx = 0; idx < vf_work->mc->count; idx++) {
> mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
> - mbx.xcast.data.mac = xaddr->addr;
> + mbx.xcast.data.mac = vf_work->mc->mc[idx];
> nicvf_send_msg_to_pf(nic, &mbx);
> -
> - /* after receiving ACK from PF release memory */
> - list_del(&xaddr->list);
> - kfree(xaddr);
> - vf_work->mc->count--;
> }
> kfree(vf_work->mc);
> }
> @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
> mode |= BGX_XCAST_MCAST_FILTER;
> /* here we need to copy mc addrs */
> if (netdev_mc_count(netdev)) {
> - struct xcast_addr *xaddr;
> -
> - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
> - INIT_LIST_HEAD(&mc_list->list);
> + mc_list = kmalloc(offsetof(typeof(*mc_list),
> + mc[netdev_mc_count(netdev)]),
> + GFP_ATOMIC);
> + if (unlikely(!mc_list))
> + return;
> + mc_list->count = 0;
> netdev_hw_addr_list_for_each(ha, &netdev->mc) {
> - xaddr = kmalloc(sizeof(*xaddr),
> - GFP_ATOMIC);
> - xaddr->addr =
> + mc_list->mc[mc_list->count] =
> ether_addr_to_u64(ha->addr);
> - list_add_tail(&xaddr->list,
> - &mc_list->list);
> mc_list->count++;
> }
> }
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4] net: thunderx: rework mac addresses list to u64 array
2018-04-06 14:04 ` Vadim Lomovtsev
@ 2018-04-06 19:53 ` Vadim Lomovtsev
-1 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 19:53 UTC (permalink / raw)
To: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, davem
Cc: dnelson, robin.murphy, hch, gustavo, Vadim Lomovtsev
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
It is too expensive to pass u64 values via linked list, instead
allocate array for them by overall number of mac addresses from netdev.
This eventually removes multiple kmalloc() calls, aviod memory
fragmentation and allow to put single null check on kmalloc
return value in order to prevent a potential null pointer dereference.
Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
Changes from v1 to v2:
- C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
Changes from v2 to v3:
- update commit description with 'Reported-by: Dan Carpenter';
- update size calculations for mc list to offsetof() call
instead of explicit arithmetic;
Changes from v3 to v4:
- change loop control variable type from u8 to int, accordingly
to mc_count size;
---
drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
2 files changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
index 5fc46c5..448d1fa 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -265,14 +265,9 @@ struct nicvf_drv_stats {
struct cavium_ptp;
-struct xcast_addr {
- struct list_head list;
- u64 addr;
-};
-
struct xcast_addr_list {
- struct list_head list;
int count;
+ u64 mc[];
};
struct nicvf_work {
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31f..6bd5658 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
work.work);
struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
union nic_mbx mbx = {};
- struct xcast_addr *xaddr, *next;
+ int idx = 0;
if (!vf_work)
return;
@@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
/* check if we have any specific MACs to be added to PF DMAC filter */
if (vf_work->mc) {
/* now go through kernel list of MACs and add them one by one */
- list_for_each_entry_safe(xaddr, next,
- &vf_work->mc->list, list) {
+ for (idx = 0; idx < vf_work->mc->count; idx++) {
mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
- mbx.xcast.data.mac = xaddr->addr;
+ mbx.xcast.data.mac = vf_work->mc->mc[idx];
nicvf_send_msg_to_pf(nic, &mbx);
-
- /* after receiving ACK from PF release memory */
- list_del(&xaddr->list);
- kfree(xaddr);
- vf_work->mc->count--;
}
kfree(vf_work->mc);
}
@@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
mode |= BGX_XCAST_MCAST_FILTER;
/* here we need to copy mc addrs */
if (netdev_mc_count(netdev)) {
- struct xcast_addr *xaddr;
-
- mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
- INIT_LIST_HEAD(&mc_list->list);
+ mc_list = kmalloc(offsetof(typeof(*mc_list),
+ mc[netdev_mc_count(netdev)]),
+ GFP_ATOMIC);
+ if (unlikely(!mc_list))
+ return;
+ mc_list->count = 0;
netdev_hw_addr_list_for_each(ha, &netdev->mc) {
- xaddr = kmalloc(sizeof(*xaddr),
- GFP_ATOMIC);
- xaddr->addr =
+ mc_list->mc[mc_list->count] =
ether_addr_to_u64(ha->addr);
- list_add_tail(&xaddr->list,
- &mc_list->list);
mc_list->count++;
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v4] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-06 19:53 ` Vadim Lomovtsev
0 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 19:53 UTC (permalink / raw)
To: linux-arm-kernel
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
It is too expensive to pass u64 values via linked list, instead
allocate array for them by overall number of mac addresses from netdev.
This eventually removes multiple kmalloc() calls, aviod memory
fragmentation and allow to put single null check on kmalloc
return value in order to prevent a potential null pointer dereference.
Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
Changes from v1 to v2:
- C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
Changes from v2 to v3:
- update commit description with 'Reported-by: Dan Carpenter';
- update size calculations for mc list to offsetof() call
instead of explicit arithmetic;
Changes from v3 to v4:
- change loop control variable type from u8 to int, accordingly
to mc_count size;
---
drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
2 files changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
index 5fc46c5..448d1fa 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -265,14 +265,9 @@ struct nicvf_drv_stats {
struct cavium_ptp;
-struct xcast_addr {
- struct list_head list;
- u64 addr;
-};
-
struct xcast_addr_list {
- struct list_head list;
int count;
+ u64 mc[];
};
struct nicvf_work {
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31f..6bd5658 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
work.work);
struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
union nic_mbx mbx = {};
- struct xcast_addr *xaddr, *next;
+ int idx = 0;
if (!vf_work)
return;
@@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
/* check if we have any specific MACs to be added to PF DMAC filter */
if (vf_work->mc) {
/* now go through kernel list of MACs and add them one by one */
- list_for_each_entry_safe(xaddr, next,
- &vf_work->mc->list, list) {
+ for (idx = 0; idx < vf_work->mc->count; idx++) {
mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
- mbx.xcast.data.mac = xaddr->addr;
+ mbx.xcast.data.mac = vf_work->mc->mc[idx];
nicvf_send_msg_to_pf(nic, &mbx);
-
- /* after receiving ACK from PF release memory */
- list_del(&xaddr->list);
- kfree(xaddr);
- vf_work->mc->count--;
}
kfree(vf_work->mc);
}
@@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
mode |= BGX_XCAST_MCAST_FILTER;
/* here we need to copy mc addrs */
if (netdev_mc_count(netdev)) {
- struct xcast_addr *xaddr;
-
- mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
- INIT_LIST_HEAD(&mc_list->list);
+ mc_list = kmalloc(offsetof(typeof(*mc_list),
+ mc[netdev_mc_count(netdev)]),
+ GFP_ATOMIC);
+ if (unlikely(!mc_list))
+ return;
+ mc_list->count = 0;
netdev_hw_addr_list_for_each(ha, &netdev->mc) {
- xaddr = kmalloc(sizeof(*xaddr),
- GFP_ATOMIC);
- xaddr->addr =
+ mc_list->mc[mc_list->count] =
ether_addr_to_u64(ha->addr);
- list_add_tail(&xaddr->list,
- &mc_list->list);
mc_list->count++;
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v4] net: thunderx: rework mac addresses list to u64 array
2018-04-06 19:53 ` Vadim Lomovtsev
@ 2018-04-08 16:42 ` David Miller
-1 siblings, 0 replies; 42+ messages in thread
From: David Miller @ 2018-04-08 16:42 UTC (permalink / raw)
To: Vadim.Lomovtsev
Cc: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, dnelson, robin.murphy, hch, gustavo,
Vadim.Lomovtsev
From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
Date: Fri, 6 Apr 2018 12:53:54 -0700
> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> work.work);
> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + int idx = 0;
No need to initialize idx.
> + for (idx = 0; idx < vf_work->mc->count; idx++) {
As it is always explicitly initialized at, and only used inside of,
this loop.
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v4] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-08 16:42 ` David Miller
0 siblings, 0 replies; 42+ messages in thread
From: David Miller @ 2018-04-08 16:42 UTC (permalink / raw)
To: linux-arm-kernel
From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
Date: Fri, 6 Apr 2018 12:53:54 -0700
> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> work.work);
> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + int idx = 0;
No need to initialize idx.
> + for (idx = 0; idx < vf_work->mc->count; idx++) {
As it is always explicitly initialized at, and only used inside of,
this loop.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v4] net: thunderx: rework mac addresses list to u64 array
2018-04-08 16:42 ` David Miller
@ 2018-04-09 9:42 ` Vadim Lomovtsev
-1 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-09 9:42 UTC (permalink / raw)
To: David Miller
Cc: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, dnelson, robin.murphy, hch, gustavo,
Vadim.Lomovtsev
On Sun, Apr 08, 2018 at 12:42:00PM -0400, David Miller wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> Date: Fri, 6 Apr 2018 12:53:54 -0700
>
> > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > work.work);
> > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > union nic_mbx mbx = {};
> > - struct xcast_addr *xaddr, *next;
> > + int idx = 0;
>
> No need to initialize idx.
>
> > + for (idx = 0; idx < vf_work->mc->count; idx++) {
>
> As it is always explicitly initialized at, and only used inside of,
> this loop.
Ok, will post next version shortly.
Thanks for your time.
Vadim
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v4] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-09 9:42 ` Vadim Lomovtsev
0 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-09 9:42 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Apr 08, 2018 at 12:42:00PM -0400, David Miller wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> Date: Fri, 6 Apr 2018 12:53:54 -0700
>
> > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > work.work);
> > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > union nic_mbx mbx = {};
> > - struct xcast_addr *xaddr, *next;
> > + int idx = 0;
>
> No need to initialize idx.
>
> > + for (idx = 0; idx < vf_work->mc->count; idx++) {
>
> As it is always explicitly initialized at, and only used inside of,
> this loop.
Ok, will post next version shortly.
Thanks for your time.
Vadim
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v5] net: thunderx: rework mac addresses list to u64 array
2018-04-06 19:53 ` Vadim Lomovtsev
@ 2018-04-09 13:24 ` Vadim Lomovtsev
-1 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-09 13:24 UTC (permalink / raw)
To: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, davem
Cc: dnelson, robin.murphy, hch, gustavo, Vadim Lomovtsev
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
It is too expensive to pass u64 values via linked list, instead
allocate array for them by overall number of mac addresses from netdev.
This eventually removes multiple kmalloc() calls, aviod memory
fragmentation and allow to put single null check on kmalloc
return value in order to prevent a potential null pointer dereference.
Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
Changes from v1 to v2:
- C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
Changes from v2 to v3:
- update commit description with 'Reported-by: Dan Carpenter';
- update size calculations for mc list to offsetof() call
instead of explicit arithmetic;
Changes from v3 to v4:
- change loop control variable type from u8 to int, accordingly
to mc_count size;
Changes from v4 to v5:
- remove explicit initialization of 'idx' variable as per review comment;
---
drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
2 files changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
index 5fc46c5a4f36..448d1fafc827 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -265,14 +265,9 @@ struct nicvf_drv_stats {
struct cavium_ptp;
-struct xcast_addr {
- struct list_head list;
- u64 addr;
-};
-
struct xcast_addr_list {
- struct list_head list;
int count;
+ u64 mc[];
};
struct nicvf_work {
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31fef729..707db3304396 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
work.work);
struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
union nic_mbx mbx = {};
- struct xcast_addr *xaddr, *next;
+ int idx;
if (!vf_work)
return;
@@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
/* check if we have any specific MACs to be added to PF DMAC filter */
if (vf_work->mc) {
/* now go through kernel list of MACs and add them one by one */
- list_for_each_entry_safe(xaddr, next,
- &vf_work->mc->list, list) {
+ for (idx = 0; idx < vf_work->mc->count; idx++) {
mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
- mbx.xcast.data.mac = xaddr->addr;
+ mbx.xcast.data.mac = vf_work->mc->mc[idx];
nicvf_send_msg_to_pf(nic, &mbx);
-
- /* after receiving ACK from PF release memory */
- list_del(&xaddr->list);
- kfree(xaddr);
- vf_work->mc->count--;
}
kfree(vf_work->mc);
}
@@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
mode |= BGX_XCAST_MCAST_FILTER;
/* here we need to copy mc addrs */
if (netdev_mc_count(netdev)) {
- struct xcast_addr *xaddr;
-
- mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
- INIT_LIST_HEAD(&mc_list->list);
+ mc_list = kmalloc(offsetof(typeof(*mc_list),
+ mc[netdev_mc_count(netdev)]),
+ GFP_ATOMIC);
+ if (unlikely(!mc_list))
+ return;
+ mc_list->count = 0;
netdev_hw_addr_list_for_each(ha, &netdev->mc) {
- xaddr = kmalloc(sizeof(*xaddr),
- GFP_ATOMIC);
- xaddr->addr =
+ mc_list->mc[mc_list->count] =
ether_addr_to_u64(ha->addr);
- list_add_tail(&xaddr->list,
- &mc_list->list);
mc_list->count++;
}
}
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v5] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-09 13:24 ` Vadim Lomovtsev
0 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-09 13:24 UTC (permalink / raw)
To: linux-arm-kernel
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
It is too expensive to pass u64 values via linked list, instead
allocate array for them by overall number of mac addresses from netdev.
This eventually removes multiple kmalloc() calls, aviod memory
fragmentation and allow to put single null check on kmalloc
return value in order to prevent a potential null pointer dereference.
Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
Changes from v1 to v2:
- C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
Changes from v2 to v3:
- update commit description with 'Reported-by: Dan Carpenter';
- update size calculations for mc list to offsetof() call
instead of explicit arithmetic;
Changes from v3 to v4:
- change loop control variable type from u8 to int, accordingly
to mc_count size;
Changes from v4 to v5:
- remove explicit initialization of 'idx' variable as per review comment;
---
drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
2 files changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
index 5fc46c5a4f36..448d1fafc827 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -265,14 +265,9 @@ struct nicvf_drv_stats {
struct cavium_ptp;
-struct xcast_addr {
- struct list_head list;
- u64 addr;
-};
-
struct xcast_addr_list {
- struct list_head list;
int count;
+ u64 mc[];
};
struct nicvf_work {
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31fef729..707db3304396 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
work.work);
struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
union nic_mbx mbx = {};
- struct xcast_addr *xaddr, *next;
+ int idx;
if (!vf_work)
return;
@@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
/* check if we have any specific MACs to be added to PF DMAC filter */
if (vf_work->mc) {
/* now go through kernel list of MACs and add them one by one */
- list_for_each_entry_safe(xaddr, next,
- &vf_work->mc->list, list) {
+ for (idx = 0; idx < vf_work->mc->count; idx++) {
mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
- mbx.xcast.data.mac = xaddr->addr;
+ mbx.xcast.data.mac = vf_work->mc->mc[idx];
nicvf_send_msg_to_pf(nic, &mbx);
-
- /* after receiving ACK from PF release memory */
- list_del(&xaddr->list);
- kfree(xaddr);
- vf_work->mc->count--;
}
kfree(vf_work->mc);
}
@@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
mode |= BGX_XCAST_MCAST_FILTER;
/* here we need to copy mc addrs */
if (netdev_mc_count(netdev)) {
- struct xcast_addr *xaddr;
-
- mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
- INIT_LIST_HEAD(&mc_list->list);
+ mc_list = kmalloc(offsetof(typeof(*mc_list),
+ mc[netdev_mc_count(netdev)]),
+ GFP_ATOMIC);
+ if (unlikely(!mc_list))
+ return;
+ mc_list->count = 0;
netdev_hw_addr_list_for_each(ha, &netdev->mc) {
- xaddr = kmalloc(sizeof(*xaddr),
- GFP_ATOMIC);
- xaddr->addr =
+ mc_list->mc[mc_list->count] =
ether_addr_to_u64(ha->addr);
- list_add_tail(&xaddr->list,
- &mc_list->list);
mc_list->count++;
}
}
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v5] net: thunderx: rework mac addresses list to u64 array
2018-04-09 13:24 ` Vadim Lomovtsev
@ 2018-04-09 15:00 ` David Miller
-1 siblings, 0 replies; 42+ messages in thread
From: David Miller @ 2018-04-09 15:00 UTC (permalink / raw)
To: Vadim.Lomovtsev
Cc: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, dnelson, robin.murphy, hch, gustavo,
Vadim.Lomovtsev
From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
Date: Mon, 9 Apr 2018 06:24:48 -0700
> From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>
> It is too expensive to pass u64 values via linked list, instead
> allocate array for them by overall number of mac addresses from netdev.
>
> This eventually removes multiple kmalloc() calls, aviod memory
> fragmentation and allow to put single null check on kmalloc
> return value in order to prevent a potential null pointer dereference.
>
> Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
Applied, thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v5] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-09 15:00 ` David Miller
0 siblings, 0 replies; 42+ messages in thread
From: David Miller @ 2018-04-09 15:00 UTC (permalink / raw)
To: linux-arm-kernel
From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
Date: Mon, 9 Apr 2018 06:24:48 -0700
> From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>
> It is too expensive to pass u64 values via linked list, instead
> allocate array for them by overall number of mac addresses from netdev.
>
> This eventually removes multiple kmalloc() calls, aviod memory
> fragmentation and allow to put single null check on kmalloc
> return value in order to prevent a potential null pointer dereference.
>
> Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
Applied, thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] net: thunderx: rework mac addresses list to u64 array
2018-04-06 11:14 ` Vadim Lomovtsev
@ 2018-04-06 15:06 ` David Miller
-1 siblings, 0 replies; 42+ messages in thread
From: David Miller @ 2018-04-06 15:06 UTC (permalink / raw)
To: Vadim.Lomovtsev
Cc: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, dnelson, gustavo, Vadim.Lomovtsev
From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
Date: Fri, 6 Apr 2018 04:14:25 -0700
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> index 5fc46c5a4f36..448d1fafc827 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
>
> struct cavium_ptp;
>
> -struct xcast_addr {
> - struct list_head list;
> - u64 addr;
> -};
> -
> struct xcast_addr_list {
> - struct list_head list;
> int count;
> + u64 mc[];
> };
>
> struct nicvf_work {
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index 1e9a31fef729..a26d8bc92e01 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> work.work);
> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + u8 idx = 0;
^^^^^^^^^^^
>
> if (!vf_work)
> return;
> @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> /* check if we have any specific MACs to be added to PF DMAC filter */
> if (vf_work->mc) {
> /* now go through kernel list of MACs and add them one by one */
> - list_for_each_entry_safe(xaddr, next,
> - &vf_work->mc->list, list) {
> + for (idx = 0; idx < vf_work->mc->count; idx++) {
vf_work->mx->count is an 'int' therefore 'idx' should be declared 'int' as well,
not a 'u8'.
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v2] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-06 15:06 ` David Miller
0 siblings, 0 replies; 42+ messages in thread
From: David Miller @ 2018-04-06 15:06 UTC (permalink / raw)
To: linux-arm-kernel
From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
Date: Fri, 6 Apr 2018 04:14:25 -0700
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> index 5fc46c5a4f36..448d1fafc827 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
>
> struct cavium_ptp;
>
> -struct xcast_addr {
> - struct list_head list;
> - u64 addr;
> -};
> -
> struct xcast_addr_list {
> - struct list_head list;
> int count;
> + u64 mc[];
> };
>
> struct nicvf_work {
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index 1e9a31fef729..a26d8bc92e01 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> work.work);
> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + u8 idx = 0;
^^^^^^^^^^^
>
> if (!vf_work)
> return;
> @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> /* check if we have any specific MACs to be added to PF DMAC filter */
> if (vf_work->mc) {
> /* now go through kernel list of MACs and add them one by one */
> - list_for_each_entry_safe(xaddr, next,
> - &vf_work->mc->list, list) {
> + for (idx = 0; idx < vf_work->mc->count; idx++) {
vf_work->mx->count is an 'int' therefore 'idx' should be declared 'int' as well,
not a 'u8'.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2] net: thunderx: rework mac addresses list to u64 array
2018-04-06 15:06 ` David Miller
@ 2018-04-06 15:14 ` Vadim Lomovtsev
-1 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 15:14 UTC (permalink / raw)
To: David Miller
Cc: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, dnelson, gustavo, Vadim.Lomovtsev
On Fri, Apr 06, 2018 at 11:06:03AM -0400, David Miller wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> Date: Fri, 6 Apr 2018 04:14:25 -0700
>
> > diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> > index 5fc46c5a4f36..448d1fafc827 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nic.h
> > +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> > @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
> >
> > struct cavium_ptp;
> >
> > -struct xcast_addr {
> > - struct list_head list;
> > - u64 addr;
> > -};
> > -
> > struct xcast_addr_list {
> > - struct list_head list;
> > int count;
> > + u64 mc[];
> > };
> >
> > struct nicvf_work {
> > diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > index 1e9a31fef729..a26d8bc92e01 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > work.work);
> > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > union nic_mbx mbx = {};
> > - struct xcast_addr *xaddr, *next;
> > + u8 idx = 0;
> ^^^^^^^^^^^
>
> >
> > if (!vf_work)
> > return;
> > @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > /* check if we have any specific MACs to be added to PF DMAC filter */
> > if (vf_work->mc) {
> > /* now go through kernel list of MACs and add them one by one */
> > - list_for_each_entry_safe(xaddr, next,
> > - &vf_work->mc->list, list) {
> > + for (idx = 0; idx < vf_work->mc->count; idx++) {
>
> vf_work->mx->count is an 'int' therefore 'idx' should be declared 'int' as well,
> not a 'u8'.
My bad, sorry.
Will post v4 shortly then.
WBR,
Vadim
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v2] net: thunderx: rework mac addresses list to u64 array
@ 2018-04-06 15:14 ` Vadim Lomovtsev
0 siblings, 0 replies; 42+ messages in thread
From: Vadim Lomovtsev @ 2018-04-06 15:14 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 06, 2018 at 11:06:03AM -0400, David Miller wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> Date: Fri, 6 Apr 2018 04:14:25 -0700
>
> > diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> > index 5fc46c5a4f36..448d1fafc827 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nic.h
> > +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> > @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
> >
> > struct cavium_ptp;
> >
> > -struct xcast_addr {
> > - struct list_head list;
> > - u64 addr;
> > -};
> > -
> > struct xcast_addr_list {
> > - struct list_head list;
> > int count;
> > + u64 mc[];
> > };
> >
> > struct nicvf_work {
> > diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > index 1e9a31fef729..a26d8bc92e01 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > work.work);
> > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > union nic_mbx mbx = {};
> > - struct xcast_addr *xaddr, *next;
> > + u8 idx = 0;
> ^^^^^^^^^^^
>
> >
> > if (!vf_work)
> > return;
> > @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > /* check if we have any specific MACs to be added to PF DMAC filter */
> > if (vf_work->mc) {
> > /* now go through kernel list of MACs and add them one by one */
> > - list_for_each_entry_safe(xaddr, next,
> > - &vf_work->mc->list, list) {
> > + for (idx = 0; idx < vf_work->mc->count; idx++) {
>
> vf_work->mx->count is an 'int' therefore 'idx' should be declared 'int' as well,
> not a 'u8'.
My bad, sorry.
Will post v4 shortly then.
WBR,
Vadim
^ permalink raw reply [flat|nested] 42+ messages in thread