Re: [PATCH net] net/flow: fix fc->percpu NULL pointer dereference
On Fri, Jun 09, 2017 at 04:23:01PM +0800, Hangbin Liu wrote: > Hi Steffen, > > BTW, If we put the check in xfrm_policy_flush(), we can prevent it earlier. > But If we put the check in flow_cache_percpu_empty(), we can prevent > other functions set fc->percpu to NULL, although not much possible : ) > > So I'm not quite sure whether we should put the check in > flow_cache_percpu_empty() or in xfrm_policy_flush(). Can't we just call xfrm_policy_fini() first and then flow_cache_fini()?
Re: [PATCH v2 7/8] net: mvmdio: add xmdio support
On Thu, Jun 08, 2017 at 09:42:21AM -0700, Florian Fainelli wrote: > On 06/08/2017 02:26 AM, Antoine Tenart wrote: > > This patch adds the xMDIO interface support in the mvmdio driver. This > > interface is used in Ethernet controllers on Marvell 370, 7k and 8k (as > > of now). The xSMI interface supported by this driver complies with the > > IEEE 802.3 clause 45 (while the SMI interface complies with the clause > > 22). The xSMI interface is used by 10GbE devices. > > In the previous version you were properly defining a new compatibles > strings for xmdio, but now you don't and instead you runtime select the > operations based on whether MII_ADDR_C45 is set in the register which is > fine from a functional perspective. > > If I get this right, the xMDIO controller is actually a superset of the > MDIO controller and has an extra MVMDIO_XSMI_ADDR_REG register to > preform C45 accesses? This is also a mistake. It's not a superset as the register offsets are different. So we can't use the same smi operations in both cases. We would need dedicated xmdio smi operations, but I don't think there is a board where a c22 PHY is connected to the xMDIO interface. I'll add smi and xsmi flags and return -ENOTSUPP based on the MII_ADDR_C45 bit and flags combination. If the need to support smi operations on the xMDIO bus arise it will be fairly easy to implement in the future. I'll rename the ops functions as well to differentiate mdio and xmdio operations. Thanks, Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx
From: Mintz, Yuval > Sent: 09 June 2017 08:52 > > From: David Laight [mailto:david.lai...@aculab.com] > > Sent: Friday, June 09, 2017 10:28 AM > > To: 'David Miller' ; Mintz, Yuval > > > > Cc: netdev@vger.kernel.org; linux-r...@vger.kernel.org; Kalderon, Michal > > > > Subject: RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx > > > > From: David Miller > > > Sent: 09 June 2017 00:24 > > > > > > From: Yuval Mintz > > > Date: Thu, 8 Jun 2017 19:13:16 +0300 > > > > > > > @@ -67,6 +79,21 @@ struct qed_ll2_stats { > > > > u64 sent_bcast_pkts; > > > > }; > > > > > > > > +struct qed_ll2_tx_pkt_info { > > > > + u8 num_of_bds; > > > > + u16 vlan; > > > > + u8 bd_flags; > > > > + u16 l4_hdr_offset_w;/* from start of packet */ > > > > + enum qed_ll2_tx_dest tx_dest; > > > > + enum qed_ll2_roce_flavor_type qed_roce_flavor; > > > > + dma_addr_t first_frag; > > > > + u16 first_frag_len; > > > > + bool enable_ip_cksum; > > > > + bool enable_l4_cksum; > > > > + bool calc_ip_len; > > > > + void *cookie; > > > > +}; > > > > + > > > > > > This layout is extremely inefficient, with lots of padding in between > > > struct members. > > > > > > Group small u8 members and u16 members together so that they consume > > > full 32-bit areas so you can eliminate all of the padding. > > > > I'd also query the use of u16 sizes/lengths, any arithmetic on u16 (and u8) > > variables is likely to generate extra code (on non-x86). > > You are using 32 bits for the 'enum' - I bet the values fit in 8 bits, so > > aren't > > really worried about size. > > > > If size did matter you can easily get the above down to 32 bytes. > > You're right, and that's exactly the point - since this is not data-path > critical > I don't see why the size/efficiency should matter [greatly]. It is just good practise so that it happens automatically when it does matter. Just swapping 'vlan' and 'bd_flags' would make it look much better. David
Re: [PATCH net] net/flow: fix fc->percpu NULL pointer dereference
Hi Steffen, BTW, If we put the check in xfrm_policy_flush(), we can prevent it earlier. But If we put the check in flow_cache_percpu_empty(), we can prevent other functions set fc->percpu to NULL, although not much possible : ) So I'm not quite sure whether we should put the check in flow_cache_percpu_empty() or in xfrm_policy_flush(). Do you have any suggestion? Thanks Hangbin 2017-06-09 16:13 GMT+08:00 Hangbin Liu : > Now we will force to do garbage collection if any policy removed in > xfrm_policy_flush(). But during xfrm_net_exit(). We call flow_cache_fini() > first and set set fc->percpu to NULL. Then after we call xfrm_policy_fini() > -> frxm_policy_flush() -> flow_cache_flush(), we will get NULL pointer > dereference when check percpu_empty. The code path looks like: > > flow_cache_fini() > - fc->percpu = NULL > xfrm_policy_fini() > - xfrm_policy_flush() > - xfrm_garbage_collect() > - flow_cache_flush() > - flow_cache_percpu_empty() > - fcp = per_cpu_ptr(fc->percpu, cpu) > > To reproduce, just add ipsec in netns and then remove the netns. > > Fixes: 35db06912189 ("xfrm: do the garbage collection after flushing policy") > Signed-off-by: Hangbin Liu > --- > net/core/flow.c | 11 +++ > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/net/core/flow.c b/net/core/flow.c > index f7f5d19..321fc53 100644 > --- a/net/core/flow.c > +++ b/net/core/flow.c > @@ -332,10 +332,13 @@ static int flow_cache_percpu_empty(struct flow_cache > *fc, int cpu) > struct flow_cache_percpu *fcp; > unsigned int i; > > - fcp = per_cpu_ptr(fc->percpu, cpu); > - for (i = 0; i < flow_cache_hash_size(fc); i++) > - if (!hlist_empty(&fcp->hash_table[i])) > - return 0; > + if (fc->percpu) { > + fcp = per_cpu_ptr(fc->percpu, cpu); > + for (i = 0; i < flow_cache_hash_size(fc); i++) > + if (!hlist_empty(&fcp->hash_table[i])) > + return 0; > + } > + > return 1; > } > > -- > 2.5.5 >
Re: [PATCH 1/5] net: mvpp2: remove mvpp2_bm_cookie_{build,pool_get}
Hi Thomas, [auto build test ERROR on net-next/master] [also build test ERROR on v4.12-rc4 next-20170608] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Thomas-Petazzoni/net-mvpp2-fixes-and-cleanups/20170609-083211 config: arm64-defconfig (attached as .config) compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm64 Note: the linux-review/Thomas-Petazzoni/net-mvpp2-fixes-and-cleanups/20170609-083211 HEAD 5a6bfd0022913d8ffcfd0eae9235b9bee844cf53 builds fine. It only hurts bisectibility. All errors (new ones prefixed by >>): drivers/net/ethernet/marvell/mvpp2.c: In function 'mvpp2_bm_cookie_pool_set': >> drivers/net/ethernet/marvell/mvpp2.c:3913:26: error: >> 'MVPP2_BM_COOKIE_POOL_OFFS' undeclared (first use in this function) bm = cookie & ~(0xFF << MVPP2_BM_COOKIE_POOL_OFFS); ^ drivers/net/ethernet/marvell/mvpp2.c:3913:26: note: each undeclared identifier is reported only once for each function it appears in vim +/MVPP2_BM_COOKIE_POOL_OFFS +3913 drivers/net/ethernet/marvell/mvpp2.c 3f518509 Marcin Wojtas 2014-07-10 3907 3f518509 Marcin Wojtas 2014-07-10 3908 /* Set pool number in a BM cookie */ 3f518509 Marcin Wojtas 2014-07-10 3909 static inline u32 mvpp2_bm_cookie_pool_set(u32 cookie, int pool) 3f518509 Marcin Wojtas 2014-07-10 3910 { 3f518509 Marcin Wojtas 2014-07-10 3911 u32 bm; 3f518509 Marcin Wojtas 2014-07-10 3912 3f518509 Marcin Wojtas 2014-07-10 @3913 bm = cookie & ~(0xFF << MVPP2_BM_COOKIE_POOL_OFFS); 3f518509 Marcin Wojtas 2014-07-10 3914 bm |= ((pool & 0xFF) << MVPP2_BM_COOKIE_POOL_OFFS); 3f518509 Marcin Wojtas 2014-07-10 3915 3f518509 Marcin Wojtas 2014-07-10 3916 return bm; :: The code at line 3913 was first introduced by commit :: 3f518509dedc99f0b755d2ce68d24f610e3a005a ethernet: Add new driver for Marvell Armada 375 network unit :: TO: Marcin Wojtas :: CC: David S. Miller --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
[PATCH net] net/flow: fix fc->percpu NULL pointer dereference
Now we will force to do garbage collection if any policy removed in xfrm_policy_flush(). But during xfrm_net_exit(). We call flow_cache_fini() first and set set fc->percpu to NULL. Then after we call xfrm_policy_fini() -> frxm_policy_flush() -> flow_cache_flush(), we will get NULL pointer dereference when check percpu_empty. The code path looks like: flow_cache_fini() - fc->percpu = NULL xfrm_policy_fini() - xfrm_policy_flush() - xfrm_garbage_collect() - flow_cache_flush() - flow_cache_percpu_empty() - fcp = per_cpu_ptr(fc->percpu, cpu) To reproduce, just add ipsec in netns and then remove the netns. Fixes: 35db06912189 ("xfrm: do the garbage collection after flushing policy") Signed-off-by: Hangbin Liu --- net/core/flow.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/net/core/flow.c b/net/core/flow.c index f7f5d19..321fc53 100644 --- a/net/core/flow.c +++ b/net/core/flow.c @@ -332,10 +332,13 @@ static int flow_cache_percpu_empty(struct flow_cache *fc, int cpu) struct flow_cache_percpu *fcp; unsigned int i; - fcp = per_cpu_ptr(fc->percpu, cpu); - for (i = 0; i < flow_cache_hash_size(fc); i++) - if (!hlist_empty(&fcp->hash_table[i])) - return 0; + if (fc->percpu) { + fcp = per_cpu_ptr(fc->percpu, cpu); + for (i = 0; i < flow_cache_hash_size(fc); i++) + if (!hlist_empty(&fcp->hash_table[i])) + return 0; + } + return 1; } -- 2.5.5
Re: [PATCH] wireless: wlcore: spi: remove unnecessary variable
"Gustavo A. R. Silva" writes: > Remove unnecessary variable and refactor the code. > > Addresses-Coverity-ID: 1365000 > Signed-off-by: Gustavo A. R. Silva I'll remove "wireless:" from the prefix. -- Kalle Valo
RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx
> -Original Message- > From: David Laight [mailto:david.lai...@aculab.com] > Sent: Friday, June 09, 2017 10:28 AM > To: 'David Miller' ; Mintz, Yuval > > Cc: netdev@vger.kernel.org; linux-r...@vger.kernel.org; Kalderon, Michal > > Subject: RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx > > From: David Miller > > Sent: 09 June 2017 00:24 > > > > From: Yuval Mintz > > Date: Thu, 8 Jun 2017 19:13:16 +0300 > > > > > @@ -67,6 +79,21 @@ struct qed_ll2_stats { > > > u64 sent_bcast_pkts; > > > }; > > > > > > +struct qed_ll2_tx_pkt_info { > > > + u8 num_of_bds; > > > + u16 vlan; > > > + u8 bd_flags; > > > + u16 l4_hdr_offset_w;/* from start of packet */ > > > + enum qed_ll2_tx_dest tx_dest; > > > + enum qed_ll2_roce_flavor_type qed_roce_flavor; > > > + dma_addr_t first_frag; > > > + u16 first_frag_len; > > > + bool enable_ip_cksum; > > > + bool enable_l4_cksum; > > > + bool calc_ip_len; > > > + void *cookie; > > > +}; > > > + > > > > This layout is extremely inefficient, with lots of padding in between > > struct members. > > > > Group small u8 members and u16 members together so that they consume > > full 32-bit areas so you can eliminate all of the padding. > > I'd also query the use of u16 sizes/lengths, any arithmetic on u16 (and u8) > variables is likely to generate extra code (on non-x86). > You are using 32 bits for the 'enum' - I bet the values fit in 8 bits, so > aren't > really worried about size. > > If size did matter you can easily get the above down to 32 bytes. You're right, and that's exactly the point - since this is not data-path critical I don't see why the size/efficiency should matter [greatly].
RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx
From: David Miller > Sent: 09 June 2017 00:24 > > From: Yuval Mintz > Date: Thu, 8 Jun 2017 19:13:16 +0300 > > > @@ -67,6 +79,21 @@ struct qed_ll2_stats { > > u64 sent_bcast_pkts; > > }; > > > > +struct qed_ll2_tx_pkt_info { > > + u8 num_of_bds; > > + u16 vlan; > > + u8 bd_flags; > > + u16 l4_hdr_offset_w;/* from start of packet */ > > + enum qed_ll2_tx_dest tx_dest; > > + enum qed_ll2_roce_flavor_type qed_roce_flavor; > > + dma_addr_t first_frag; > > + u16 first_frag_len; > > + bool enable_ip_cksum; > > + bool enable_l4_cksum; > > + bool calc_ip_len; > > + void *cookie; > > +}; > > + > > This layout is extremely inefficient, with lots of padding in between > struct members. > > Group small u8 members and u16 members together so that they consume > full 32-bit areas so you can eliminate all of the padding. I'd also query the use of u16 sizes/lengths, any arithmetic on u16 (and u8) variables is likely to generate extra code (on non-x86). You are using 32 bits for the 'enum' - I bet the values fit in 8 bits, so aren't really worried about size. If size did matter you can easily get the above down to 32 bytes. David
RE: [B.A.T.M.A.N.] [PATCH net] net: batman-adv: Fix possible memleaks when fail to register_netdevice
> From: Sven Eckelmann [mailto:s...@narfation.org] > Sent: Friday, June 9, 2017 3:23 PM > Subject: Re: [B.A.T.M.A.N.] [PATCH net] net: batman-adv: Fix possible memleaks > when fail to register_netdevice > > On Dienstag, 25. April 2017 20:03:20 CEST gfree.w...@foxmail.com wrote: > > From: Gao Feng > > > > Because the func batadv_softif_init_late allocate some resources and > > it would be invoked in register_netdevice. So we need to invoke the > > func batadv_softif_free instead of free_netdev to cleanup when fail to > > register_netdevice. > > > > Signed-off-by: Gao Feng > > --- > > net/batman-adv/soft-interface.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/batman-adv/soft-interface.c > > b/net/batman-adv/soft-interface.c index d042c99..90bf990 100644 > > --- a/net/batman-adv/soft-interface.c > > +++ b/net/batman-adv/soft-interface.c > > @@ -1011,7 +1011,7 @@ struct net_device *batadv_softif_create(struct net > *net, const char *name) > > if (ret < 0) { > > pr_err("Unable to register the batman interface '%s': %i\n", > >name, ret); > > - free_netdev(soft_iface); > > + batadv_softif_free(soft_iface); > > return NULL; > > } > > It looks to me like this change is invalid after David's change [1]. Can you > confirm that? > > Thanks, > Sven > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=cf1 > 24db566e6b036b8bcbe8decbed740bdfac8c6 [Gao Feng] yes, this change is unnecessary. Best Regards Feng
Re: [B.A.T.M.A.N.] [PATCH net] net: batman-adv: Fix possible memleaks when fail to register_netdevice
On Dienstag, 25. April 2017 20:03:20 CEST gfree.w...@foxmail.com wrote: > From: Gao Feng > > Because the func batadv_softif_init_late allocate some resources and > it would be invoked in register_netdevice. So we need to invoke the > func batadv_softif_free instead of free_netdev to cleanup when fail > to register_netdevice. > > Signed-off-by: Gao Feng > --- > net/batman-adv/soft-interface.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c > index d042c99..90bf990 100644 > --- a/net/batman-adv/soft-interface.c > +++ b/net/batman-adv/soft-interface.c > @@ -1011,7 +1011,7 @@ struct net_device *batadv_softif_create(struct net > *net, const char *name) > if (ret < 0) { > pr_err("Unable to register the batman interface '%s': %i\n", > name, ret); > - free_netdev(soft_iface); > + batadv_softif_free(soft_iface); > return NULL; > } It looks to me like this change is invalid after David's change [1]. Can you confirm that? Thanks, Sven [1] https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=cf124db566e6b036b8bcbe8decbed740bdfac8c6 signature.asc Description: This is a digitally signed message part.
Re: [PATCH net-next 0/8] Bug fixes in ena ethernet driver
My apologies, I was not aware. Will make sure this won't happen again. Regards, Netanel From: David Miller Sent: Friday, June 9, 2017 2:17 AM To: Belgazal, Netanel Cc: netdev@vger.kernel.org; Woodhouse, David; Machulsky, Zorik; Matushevsky, Alexander; BSHARA, Said; Wilson, Matt; Liguori, Anthony; Bshara, Nafea; Schmeilin, Evgeny Subject: Re: [PATCH net-next 0/8] Bug fixes in ena ethernet driver Two parallel patch series to the same driver and targetting the same GIT tree is extremely undesirable, please don't do this. Submit one series, and once applied submit the second series. I'm deleting all of your patches from my queue, please resubmit things properly. Thank you.
Re: [PATCH 2/2 net-next] net: stmmac: Improve documentation on AVB parameters
Hi Joao On 6/8/2017 8:02 PM, Joao Pinto wrote: This patch fixes the description of the DT AVB parameters and gives an accurate example. It was also included the base values that were used to get the example' CBS paremeter values. Signed-off-by: Joao Pinto --- Documentation/devicetree/bindings/net/stmmac.txt | 24 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt index c3a7be6..707426d 100644 --- a/Documentation/devicetree/bindings/net/stmmac.txt +++ b/Documentation/devicetree/bindings/net/stmmac.txt @@ -109,10 +109,10 @@ Optional properties: [Attention] Queue 0 is reserved for legacy traffic and so no AVB is available in this queue. - Configure Credit Base Shaper (if AVB Mode selected): - - snps,send_slope: enable Low Power Interface - - snps,idle_slope: unlock on WoL - - snps,high_credit: max write outstanding req. limit - - snps,low_credit: max read outstanding req. limit + - snps,send_slope: Send Slope Credit value + - snps,idle_slope: Idle Slope Credit value + - snps,high_credit: High Credit value + - snps,low_credit: Low Credit value - snps,priority: TX queue priority (Range: 0x0 to 0xF) Examples: @@ -143,10 +143,18 @@ Examples: queue1 { snps,avb-algorithm; - snps,send_slope = <0x1000>; - snps,idle_slope = <0x1000>; - snps,high_credit = <0x3E800>; - snps,low_credit = <0xFFC18000>; + /* +* Example AVB parameters based on: +* Allocated Bandwidth: 40% +* Maximum Frame size: 1000 bytes +* Maximum Interference size: 1500 bytes +* Port Transmit Rate: 8 +* Scaling Factor: 1024 +*/ + snps,idle_slope = <0xCCC>; + snps,send_slope = <0x1333>; + snps,high_credit = <0x4B>; Thanks for having taken care about this changes, please, as required, add a cover-letter and give more information about these values that can be tuned by user and, for example, the snps,high_credit could be as default = 0xbe4000 that is a reasonable value because comes from 1522 * 8 * 1024 and LOW credit is the two complement. ^ frame size ---> maximum is 16 Regards Peppe + snps,low_credit = <0xFFB5>; snps,priority = <0x1>; }; };
RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx
> > +struct qed_ll2_tx_pkt_info { > > + u8 num_of_bds; > > + u16 vlan; > > + u8 bd_flags; > > + u16 l4_hdr_offset_w;/* from start of packet */ > > + enum qed_ll2_tx_dest tx_dest; > > + enum qed_ll2_roce_flavor_type qed_roce_flavor; > > + dma_addr_t first_frag; > > + u16 first_frag_len; > > + bool enable_ip_cksum; > > + bool enable_l4_cksum; > > + bool calc_ip_len; > > + void *cookie; > > +}; > > + > > This layout is extremely inefficient, with lots of padding in between struct > members. > > Group small u8 members and u16 members together so that they consume > full 32-bit areas so you can eliminate all of the padding. While I can mend the holes, the total structure size doesn't really change: struct qed_ll2_tx_pkt_info { u8 num_of_bds; /* 0 1 */ /* XXX 1 byte hole, try to pack */ u16vlan; /* 2 2 */ u8 bd_flags; /* 4 1 */ /* XXX 1 byte hole, try to pack */ u16l4_hdr_offset_w; /* 6 2 */ enum qed_ll2_tx_dest tx_dest; /* 8 4 */ enum qed_ll2_roce_flavor_type qed_roce_flavor; /*12 4 */ dma_addr_t first_frag; /*16 8 */ u16first_frag_len; /*24 2 */ bool enable_ip_cksum; /*26 1 */ bool enable_l4_cksum; /*27 1 */ bool calc_ip_len; /*28 1 */ /* XXX 3 bytes hole, try to pack */ void * cookie; /*32 8 */ /* size: 40, cachelines: 1, members: 12 */ /* sum members: 35, holes: 3, sum holes: 5 */ /* last cacheline: 40 bytes */ }; Becomes: struct qed_ll2_tx_pkt_info { void * cookie; /* 0 8 */ dma_addr_t first_frag; /* 8 8 */ enum qed_ll2_tx_dest tx_dest; /*16 4 */ enum qed_ll2_roce_flavor_type qed_roce_flavor; /*20 4 */ u16vlan; /*24 2 */ u16l4_hdr_offset_w; /*26 2 */ u16first_frag_len; /*28 2 */ u8 num_of_bds; /*30 1 */ u8 bd_flags; /*31 1 */ bool enable_ip_cksum; /*32 1 */ bool enable_l4_cksum; /*33 1 */ bool calc_ip_len; /*34 1 */ /* size: 40, cachelines: 1, members: 12 */ /* padding: 5 */ /* last cacheline: 40 bytes */ }; I'm going to send the changed version in V2 as as there's no harm to it, [+ we *can* reduce the size of qed_ll2_comp_rx_data you commented for patch #2 in series] But one thing I thought of asking - do we consider layouts of relatively insignificant structures to be some golden coding standard?