All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: dsa: drop some VLAs in switch.c
@ 2018-03-13 19:50 Salvatore Mesoraca
  2018-03-13 19:58 ` Vivien Didelot
  0 siblings, 1 reply; 12+ messages in thread
From: Salvatore Mesoraca @ 2018-03-13 19:50 UTC (permalink / raw)
  To: linux-kernel
  Cc: kernel-hardening, netdev, David S. Miller, Andrew Lunn,
	Florian Fainelli, Kees Cook, Salvatore Mesoraca, Vivien Didelot

dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
---
 net/dsa/switch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index b935117..78e9897 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -136,7 +136,7 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
 {
 	const struct switchdev_obj_port_mdb *mdb = info->mdb;
 	struct switchdev_trans *trans = info->trans;
-	DECLARE_BITMAP(group, ds->num_ports);
+	DECLARE_BITMAP(group, DSA_MAX_PORTS);
 	int port;
 
 	/* Build a mask of Multicast group members */
@@ -204,7 +204,7 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds,
 {
 	const struct switchdev_obj_port_vlan *vlan = info->vlan;
 	struct switchdev_trans *trans = info->trans;
-	DECLARE_BITMAP(members, ds->num_ports);
+	DECLARE_BITMAP(members, DSA_MAX_PORTS);
 	int port;
 
 	/* Build a mask of VLAN members */
-- 
1.9.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net: dsa: drop some VLAs in switch.c
  2018-03-13 19:50 [PATCH] net: dsa: drop some VLAs in switch.c Salvatore Mesoraca
@ 2018-03-13 19:58 ` Vivien Didelot
  2018-03-13 20:06   ` Florian Fainelli
  2018-03-13 22:01   ` Salvatore Mesoraca
  0 siblings, 2 replies; 12+ messages in thread
From: Vivien Didelot @ 2018-03-13 19:58 UTC (permalink / raw)
  To: Salvatore Mesoraca, linux-kernel
  Cc: kernel-hardening, netdev, David S. Miller, Andrew Lunn,
	Florian Fainelli, Kees Cook, Salvatore Mesoraca

Hi Salvatore,

Salvatore Mesoraca <s.mesoraca16@gmail.com> writes:

> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>

NAK.

We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.


Thanks,

        Vivien

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net: dsa: drop some VLAs in switch.c
@ 2018-03-13 19:58 ` Vivien Didelot
  2018-03-13 20:06   ` Florian Fainelli
  2018-03-13 22:01   ` Salvatore Mesoraca
  0 siblings, 2 replies; 12+ messages in thread
From: Vivien Didelot @ 2018-03-13 19:58 UTC (permalink / raw)
  To: Salvatore Mesoraca, linux-kernel
  Cc: kernel-hardening, netdev, David S. Miller, Andrew Lunn,
	Florian Fainelli, Kees Cook

Hi Salvatore,

Salvatore Mesoraca <s.mesoraca16@gmail.com> writes:

> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>

NAK.

We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.


Thanks,

        Vivien

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net: dsa: drop some VLAs in switch.c
  2018-03-13 19:58 ` Vivien Didelot
@ 2018-03-13 20:06   ` Florian Fainelli
  2018-05-05 10:36     ` Salvatore Mesoraca
  2018-03-13 22:01   ` Salvatore Mesoraca
  1 sibling, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2018-03-13 20:06 UTC (permalink / raw)
  To: Vivien Didelot, Salvatore Mesoraca, linux-kernel
  Cc: kernel-hardening, netdev, David S. Miller, Andrew Lunn, Kees Cook

On 03/13/2018 12:58 PM, Vivien Didelot wrote:
> Hi Salvatore,
> 
> Salvatore Mesoraca <s.mesoraca16@gmail.com> writes:
> 
>> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
>> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
> 
> NAK.
> 
> We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
> and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.

Then this means that we need to allocate a bitmap from the heap, which
sounds a bit superfluous and could theoretically fail... not sure which
way is better, but bumping the size to DSA_MAX_PORTS definitively does
help people working on enabling -Wvla.
-- 
Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net: dsa: drop some VLAs in switch.c
  2018-03-13 19:58 ` Vivien Didelot
  2018-03-13 20:06   ` Florian Fainelli
@ 2018-03-13 22:01   ` Salvatore Mesoraca
  2018-03-14 11:24     ` David Laight
  1 sibling, 1 reply; 12+ messages in thread
From: Salvatore Mesoraca @ 2018-03-13 22:01 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: linux-kernel, Kernel Hardening, netdev, David S. Miller,
	Andrew Lunn, Florian Fainelli, Kees Cook

2018-03-13 20:58 GMT+01:00 Vivien Didelot <vivien.didelot@savoirfairelinux.com>:
> Hi Salvatore,

Hi Vivien,

> Salvatore Mesoraca <s.mesoraca16@gmail.com> writes:
>
>> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
>> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
>
> NAK.
>
> We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
> and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.

I can rewrite the patch using kmalloc.
Although, if ds->num_ports will always be less than or equal to 12, it
should be better to
just use DSA_MAX_PORTS.

Thank you,

Salvatore

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] net: dsa: drop some VLAs in switch.c
  2018-03-13 22:01   ` Salvatore Mesoraca
@ 2018-03-14 11:24     ` David Laight
  2018-03-14 12:48       ` Salvatore Mesoraca
  0 siblings, 1 reply; 12+ messages in thread
From: David Laight @ 2018-03-14 11:24 UTC (permalink / raw)
  To: 'Salvatore Mesoraca', Vivien Didelot
  Cc: linux-kernel, Kernel Hardening, netdev, David S. Miller,
	Andrew Lunn, Florian Fainelli, Kees Cook

From: Salvatore Mesoraca
> Sent: 13 March 2018 22:01
> 2018-03-13 20:58 GMT+01:00 Vivien Didelot <vivien.didelot@savoirfairelinux.com>:
> > Hi Salvatore,
> 
> Hi Vivien,
> 
> > Salvatore Mesoraca <s.mesoraca16@gmail.com> writes:
> >
> >> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
> >> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
> >>
> >> [1] https://lkml.org/lkml/2018/3/7/621
> >>
> >> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
> >
> > NAK.
> >
> > We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
> > and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.
> 
> I can rewrite the patch using kmalloc.
> Although, if ds->num_ports will always be less than or equal to 12, it
> should be better to
> just use DSA_MAX_PORTS.

Isn't using DECLARE_BITMAP() completely OTT when the maximum size is less
than the number of bits in a word?

	David


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net: dsa: drop some VLAs in switch.c
  2018-03-14 11:24     ` David Laight
@ 2018-03-14 12:48       ` Salvatore Mesoraca
  2018-03-18 14:08         ` Salvatore Mesoraca
  0 siblings, 1 reply; 12+ messages in thread
From: Salvatore Mesoraca @ 2018-03-14 12:48 UTC (permalink / raw)
  To: David Laight
  Cc: Vivien Didelot, linux-kernel, Kernel Hardening, netdev,
	David S. Miller, Andrew Lunn, Florian Fainelli, Kees Cook

2018-03-14 12:24 GMT+01:00 David Laight <David.Laight@aculab.com>:
> Isn't using DECLARE_BITMAP() completely OTT when the maximum size is less
> than the number of bits in a word?

It allocates ceiling(size/8) "unsigned long"s, so yes.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net: dsa: drop some VLAs in switch.c
  2018-03-14 12:48       ` Salvatore Mesoraca
@ 2018-03-18 14:08         ` Salvatore Mesoraca
  0 siblings, 0 replies; 12+ messages in thread
From: Salvatore Mesoraca @ 2018-03-18 14:08 UTC (permalink / raw)
  To: David Laight
  Cc: Vivien Didelot, linux-kernel, Kernel Hardening, netdev,
	David S. Miller, Andrew Lunn, Florian Fainelli, Kees Cook

2018-03-14 13:48 GMT+01:00 Salvatore Mesoraca <s.mesoraca16@gmail.com>:
> 2018-03-14 12:24 GMT+01:00 David Laight <David.Laight@aculab.com>:
>> Isn't using DECLARE_BITMAP() completely OTT when the maximum size is less
>> than the number of bits in a word?
>
> It allocates ceiling(size/8) "unsigned long"s, so yes.

Actually I meant ceiling(size/8/sizeof(unsigned long))
I'm sorry for the typo.

Salvatore

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net: dsa: drop some VLAs in switch.c
  2018-03-13 20:06   ` Florian Fainelli
@ 2018-05-05 10:36     ` Salvatore Mesoraca
  2018-05-05 15:39       ` Andrew Lunn
  0 siblings, 1 reply; 12+ messages in thread
From: Salvatore Mesoraca @ 2018-05-05 10:36 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Vivien Didelot, linux-kernel, Kernel Hardening, netdev,
	David S. Miller, Andrew Lunn, Kees Cook

2018-03-13 21:06 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>:
> On 03/13/2018 12:58 PM, Vivien Didelot wrote:
>> Hi Salvatore,
>>
>> Salvatore Mesoraca <s.mesoraca16@gmail.com> writes:
>>
>>> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
>>> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
>>>
>>> [1] https://lkml.org/lkml/2018/3/7/621
>>>
>>> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
>>
>> NAK.
>>
>> We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
>> and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.
>
> Then this means that we need to allocate a bitmap from the heap, which
> sounds a bit superfluous and could theoretically fail... not sure which
> way is better, but bumping the size to DSA_MAX_PORTS definitively does
> help people working on enabling -Wvla.

Hi Florian,

Should I consider this patch still NAKed or not?
Should I resend the patch with some modifications?

Thank you,

Salvatore

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net: dsa: drop some VLAs in switch.c
  2018-05-05 10:36     ` Salvatore Mesoraca
@ 2018-05-05 15:39       ` Andrew Lunn
  2018-05-05 18:22         ` Kees Cook
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2018-05-05 15:39 UTC (permalink / raw)
  To: Salvatore Mesoraca
  Cc: Florian Fainelli, Vivien Didelot, linux-kernel, Kernel Hardening,
	netdev, David S. Miller, Kees Cook

On Sat, May 05, 2018 at 12:36:36PM +0200, Salvatore Mesoraca wrote:
> 2018-03-13 21:06 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>:
> > On 03/13/2018 12:58 PM, Vivien Didelot wrote:
> >> Hi Salvatore,
> >>
> >> Salvatore Mesoraca <s.mesoraca16@gmail.com> writes:
> >>
> >>> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
> >>> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
> >>>
> >>> [1] https://lkml.org/lkml/2018/3/7/621
> >>>
> >>> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
> >>
> >> NAK.
> >>
> >> We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
> >> and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.
> >
> > Then this means that we need to allocate a bitmap from the heap, which
> > sounds a bit superfluous and could theoretically fail... not sure which
> > way is better, but bumping the size to DSA_MAX_PORTS definitively does
> > help people working on enabling -Wvla.
> 
> Hi Florian,
> 
> Should I consider this patch still NAKed or not?
> Should I resend the patch with some modifications?

Hi Salvatore

We have been removing all uses of DSA_MAX_PORTS. I don't particularly
like arbitrary limits on how many ports a switch can have, or how many
switches a board can have.

So i would prefer to not use DSA_MAX_PORTS here.

You could make the bitmap part of the dsa_switch structure. This is
allocated by dsa_switch_alloc() and is passed the number of ports.
Doing the allocation there means you don't need to worry about it
failing in dsa_switch_mdb_add() or dsa_switch_vlan_add().

	Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net: dsa: drop some VLAs in switch.c
  2018-05-05 15:39       ` Andrew Lunn
@ 2018-05-05 18:22         ` Kees Cook
  2018-05-05 18:51           ` Andrew Lunn
  0 siblings, 1 reply; 12+ messages in thread
From: Kees Cook @ 2018-05-05 18:22 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Salvatore Mesoraca, Florian Fainelli, Vivien Didelot, LKML,
	Kernel Hardening, Network Development, David S. Miller

On Sat, May 5, 2018 at 8:39 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Sat, May 05, 2018 at 12:36:36PM +0200, Salvatore Mesoraca wrote:
>> 2018-03-13 21:06 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>:
>> > On 03/13/2018 12:58 PM, Vivien Didelot wrote:
>> >> Hi Salvatore,
>> >>
>> >> Salvatore Mesoraca <s.mesoraca16@gmail.com> writes:
>> >>
>> >>> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
>> >>> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
>> >>>
>> >>> [1] https://lkml.org/lkml/2018/3/7/621
>> >>>
>> >>> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
>> >>
>> >> NAK.
>> >>
>> >> We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
>> >> and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.
>> >
>> > Then this means that we need to allocate a bitmap from the heap, which
>> > sounds a bit superfluous and could theoretically fail... not sure which
>> > way is better, but bumping the size to DSA_MAX_PORTS definitively does
>> > help people working on enabling -Wvla.
>>
>> Hi Florian,
>>
>> Should I consider this patch still NAKed or not?
>> Should I resend the patch with some modifications?
>
> Hi Salvatore
>
> We have been removing all uses of DSA_MAX_PORTS. I don't particularly
> like arbitrary limits on how many ports a switch can have, or how many
> switches a board can have.
>
> So i would prefer to not use DSA_MAX_PORTS here.
>
> You could make the bitmap part of the dsa_switch structure. This is
> allocated by dsa_switch_alloc() and is passed the number of ports.
> Doing the allocation there means you don't need to worry about it
> failing in dsa_switch_mdb_add() or dsa_switch_vlan_add().

Are dsa_switch_mdb_add() and dsa_switch_vlan_add() guaranteed to be
single-threaded?

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net: dsa: drop some VLAs in switch.c
  2018-05-05 18:22         ` Kees Cook
@ 2018-05-05 18:51           ` Andrew Lunn
  0 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2018-05-05 18:51 UTC (permalink / raw)
  To: Kees Cook
  Cc: Salvatore Mesoraca, Florian Fainelli, Vivien Didelot, LKML,
	Kernel Hardening, Network Development, David S. Miller

> > You could make the bitmap part of the dsa_switch structure. This is
> > allocated by dsa_switch_alloc() and is passed the number of ports.
> > Doing the allocation there means you don't need to worry about it
> > failing in dsa_switch_mdb_add() or dsa_switch_vlan_add().
> 
> Are dsa_switch_mdb_add() and dsa_switch_vlan_add() guaranteed to be
> single-threaded?

Yes, that is the interesting question here.... against each other, or
themselves?

They are called from a notifier chain. It is the same notifier chain
for both dsa_switch_mdb_add() and dsa_switch_vlan_add().

notifier_call_chain() itself appears to not provide any guarantees
about the same handler being called in parallel.

It is dsa_port_notify() which is calling the notifier_call_chain().
This is being called by both dsa_port_vlan_add() and
dsa_port_mdb_add() in dsa_slave_port_obj_add(). This is a switchdev
op. switchdev_port_obj_add_now() does have ASSERT_RTNL(); So that
should serialize everything.

     Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-05-05 18:51 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-13 19:50 [PATCH] net: dsa: drop some VLAs in switch.c Salvatore Mesoraca
2018-03-13 19:58 ` Vivien Didelot
2018-03-13 20:06   ` Florian Fainelli
2018-05-05 10:36     ` Salvatore Mesoraca
2018-05-05 15:39       ` Andrew Lunn
2018-05-05 18:22         ` Kees Cook
2018-05-05 18:51           ` Andrew Lunn
2018-03-13 22:01   ` Salvatore Mesoraca
2018-03-14 11:24     ` David Laight
2018-03-14 12:48       ` Salvatore Mesoraca
2018-03-18 14:08         ` Salvatore Mesoraca

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.