* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
From: Julien Gomes @ 2019-02-06 21:53 UTC (permalink / raw)
To: Neil Horman
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
In-Reply-To: <271b88ca-ae7c-7d07-330a-242e8ba7322d@arista.com>
On 2/6/19 1:48 PM, Julien Gomes wrote:
>
>
> On 2/6/19 1:39 PM, Neil Horman wrote:
>> On Wed, Feb 06, 2019 at 01:26:55PM -0800, Julien Gomes wrote:
>>>
>>>
>>> On 2/6/19 1:07 PM, Marcelo Ricardo Leitner wrote:
>>>> On Wed, Feb 06, 2019 at 12:48:38PM -0800, Julien Gomes wrote:
>>>>>
>>>>>
>>>>> On 2/6/19 12:37 PM, Marcelo Ricardo Leitner wrote:
>>>>>> On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
>>>>>>> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
>>>>>>> structures longer than the current definitions.
>>>>>>>
>>>>>>> This should prevent unjustified setsockopt() failures due to struct
>>>>>>> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
>>>>>>> binaries that should be compatible, but were built with later kernel
>>>>>>> uapi headers.
>>>>>>
>>>>>> Not sure if we support backwards compatibility like this?
>>>>>>
>>>>>> My issue with this change is that by doing this, application will have
>>>>>> no clue if the new bits were ignored or not and it may think that an
>>>>>> event is enabled while it is not.
>>>>>>
>>>>>> A workaround would be to do a getsockopt and check the size that was
>>>>>> returned. But then, it might as well use the right struct here in the
>>>>>> first place.
>>>>>>
>>>>>> I'm seeing current implementation as an implicitly versioned argument:
>>>>>> it will always accept setsockopt calls with an old struct (v4.11 or
>>>>>> v4.12), but if the user tries to use v3 on a v1-only system, it will
>>>>>> be rejected. Pretty much like using a newer setsockopt on an old
>>>>>> system.
>>>>>
>>>>> With the current implementation, given sources that say are supposed to
>>>>> run on a 4.9 kernel (no use of any newer field added in 4.11 or 4.12),
>>>>> we can't rebuild the exact same sources on a 4.19 kernel and still run
>>>>> them on 4.9 without messing with structures re-definition.
>>>>
>>>> Maybe what we want(ed) here then is explicit versioning, to have the 3
>>>> definitions available. Then the application is able to use, say struct
>>>> sctp_event_subscribe, and be happy with it, while there is struct
>>>> sctp_event_subscribe_v2 and struct sctp_event_subscribe_v3 there too.
>>>>
>>>> But it's too late for that now because that would break applications
>>>> already using the new fields in sctp_event_subscribe.
>>>
>>> Right.
>>>
>>>>
>>>>>
>>>>> I understand your point, but this still looks like a sort of uapi
>>>>> breakage to me.
>>>>
>>>> Not disagreeing. I really just don't know how supported that is.
>>>> Willing to know so I can pay more attention to this on future changes.
>>>>
>>>> Btw, is this the only occurrence?
>>>
>>> Can't really say, this is one I witnessed, I haven't really looked for
>>> others.
>>>
>>>>
>>>>>
>>>>>
>>>>> I also had another way to work-around this in mind, by copying optlen
>>>>> bytes and checking that any additional field (not included in the
>>>>> "current" kernel structure definition) is not set, returning EINVAL in
>>>>> such case to keep a similar to current behavior.
>>>>> The issue with this is that I didn't find a suitable (ie not totally
>>>>> arbitrary such as "twice the existing structure size") upper limit to
>>>>> optlen.
>>>>
>>>> Seems interesting. Why would it need that upper limit to optlen?
>>>>
>>>> Say struct v1 had 4 bytes, v3 now had 12. The user supplies 12 bytes
>>>> to the kernel that only knows about 4 bytes. It can check that (12-4)
>>>> bytes in the end, make sure no bit is on and use only the first 4.
>>>>
>>>> The fact that it was 12 or 200 shouldn't matter, should it? As long as
>>>> the (200-4) bytes are 0'ed, only the first 4 will be used and it
>>>> should be ok, otherwise EINVAL. No need to know how big the current
>>>> current actually is because it wouldn't be validating that here: just
>>>> that it can safely use the first 4 bytes.
>>>
>>> The upper limit concern is more regarding the call to copy_from_user
>>> with an unrestricted/unchecked value.
>> Copy_from_user should be safe to copy an arbitrary amount, the only restriction
>> is that optlen can't exceed the size of the buffer receiving the data in the
>> kernel. From that standpoint your patch is safe. However, that exposes the
>> problem of checking any tail data on the userspace buffer. That is to say, if
>> you want to ensure that any extra data that gets sent from userspace isn't
>> 'set', you would have to copy that extra data in consumable chunks and check
>> them individaully, and that screams DOS to me (i.e. imagine a user passing in a
>> 4GB buffer, and having to wait for the kernel to copy each X sized chunk,
>> looking for non-zero values).
>
> There probably is a decent compromise to find between "not accepting a
> single additional byte" and accepting several GB.
> For example how likely is it that the growth of this structure make it
> go over a page? I would hope not at all.
>
> By choosing a large but decent high limit, I think we can find a
> future-compatible compromise that doesn't rely on a preliminary
> getsockopt() just for structure trucation decision...
And I was just reminded about huge pages.
But still, my point of finding a compromise still stands.
>
>>
>>> I am not sure of how much of a risk/how exploitable this could be,
>>> that's why I cautiously wanted to limit it in the first place just in case.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Julien Gomes <julien@arista.com>
>>>>>>> ---
>>>>>>> net/sctp/socket.c | 2 +-
>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>>>>> index 9644bdc8e85c..f9717e2789da 100644
>>>>>>> --- a/net/sctp/socket.c
>>>>>>> +++ b/net/sctp/socket.c
>>>>>>> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
>>>>>>> int i;
>>>>>>>
>>>>>>> if (optlen > sizeof(struct sctp_event_subscribe))
>>>>>>> - return -EINVAL;
>>>>>>> + optlen = sizeof(struct sctp_event_subscribe);
>>>>>>>
>>>>>>> if (copy_from_user(&subscribe, optval, optlen))
>>>>>>> return -EFAULT;
>>>>>>> --
>>>>>>> 2.20.1
>>>>>>>
>>>>>
>>>
>>> --
>>> Julien Gomes
>>>
>
^ permalink raw reply
* Re: [RFC net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c
From: Andrew Lunn @ 2019-02-06 21:53 UTC (permalink / raw)
To: Moritz Fischer
Cc: linux-kernel, linux-usb, netdev, colin.king, linus.walleij,
yuehaibing, mcgrof, frowand.list, robh+dt, UNGLinuxDriver,
woojung.huh, hkallweit1, davem, f.fainelli, vivien.didelot,
moritz, alex.williams
In-Reply-To: <20190206205106.11517-1-mdf@kernel.org>
On Wed, Feb 06, 2019 at 12:51:06PM -0800, Moritz Fischer wrote:
> Move the DT based link GPIO parsing to of_mdio and let the places
> that register a fixed_phy pass in a GPIO descriptor or NULL.
>
> This allows fixed_phy on non-DT platforms to have link GPIOs, too.
>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
> drivers/net/dsa/dsa_loop.c | 2 +-
> drivers/net/ethernet/broadcom/bgmac.c | 2 +-
> drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 +-
> drivers/net/phy/fixed_phy.c | 48 ++------------------
> drivers/net/usb/lan78xx.c | 2 +-
> drivers/of/of_mdio.c | 15 +++++-
> include/linux/phy_fixed.h | 3 +-
> 7 files changed, 23 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
> index 17482ae09aa5..7f124c620092 100644
> --- a/drivers/net/dsa/dsa_loop.c
> +++ b/drivers/net/dsa/dsa_loop.c
> @@ -343,7 +343,7 @@ static int __init dsa_loop_init(void)
> unsigned int i;
>
> for (i = 0; i < NUM_FIXED_PHYS; i++)
> - phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL);
> + phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL, NULL);
>
> return mdio_driver_register(&dsa_loop_drv);
> }
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 4632dd5dbad1..bce644dec5c2 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -1446,7 +1446,7 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac)
> struct phy_device *phy_dev;
> int err;
>
> - phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
> + phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
> if (!phy_dev || IS_ERR(phy_dev)) {
> dev_err(bgmac->dev, "Failed to register fixed PHY device\n");
> return -ENODEV;
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> index 51880d83131a..7cbd737aba80 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> @@ -525,7 +525,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
> .asym_pause = 0,
> };
>
> - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
> + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
> if (!phydev || IS_ERR(phydev)) {
> dev_err(kdev, "failed to register fixed PHY device\n");
> return -ENODEV;
> diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
> index d810f914aaa4..845bd7c2065a 100644
> --- a/drivers/net/phy/fixed_phy.c
> +++ b/drivers/net/phy/fixed_phy.c
> @@ -18,6 +18,7 @@
> #include <linux/err.h>
> #include <linux/slab.h>
> #include <linux/of.h>
> +#include <linux/of_mdio.h>
> #include <linux/gpio/consumer.h>
> #include <linux/seqlock.h>
> #include <linux/idr.h>
> @@ -191,50 +192,12 @@ static void fixed_phy_del(int phy_addr)
> }
> }
>
> -#ifdef CONFIG_OF_GPIO
> -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np)
> -{
> - struct device_node *fixed_link_node;
> - struct gpio_desc *gpiod;
> -
> - if (!np)
> - return NULL;
> -
> - fixed_link_node = of_get_child_by_name(np, "fixed-link");
> - if (!fixed_link_node)
> - return NULL;
> -
> - /*
> - * As the fixed link is just a device tree node without any
> - * Linux device associated with it, we simply have obtain
> - * the GPIO descriptor from the device tree like this.
> - */
> - gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0,
> - GPIOD_IN, "mdio");
> - of_node_put(fixed_link_node);
> - if (IS_ERR(gpiod)) {
> - if (PTR_ERR(gpiod) == -EPROBE_DEFER)
> - return gpiod;
> - pr_err("error getting GPIO for fixed link %pOF, proceed without\n",
> - fixed_link_node);
> - gpiod = NULL;
> - }
> -
> - return gpiod;
> -}
> -#else
> -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np)
> -{
> - return NULL;
> -}
> -#endif
> -
> struct phy_device *fixed_phy_register(unsigned int irq,
> struct fixed_phy_status *status,
> - struct device_node *np)
> + struct device_node *np,
> + struct gpio_desc *gpiod)
> {
> struct fixed_mdio_bus *fmb = &platform_fmb;
> - struct gpio_desc *gpiod = NULL;
> struct phy_device *phy;
> int phy_addr;
> int ret;
> @@ -242,11 +205,6 @@ struct phy_device *fixed_phy_register(unsigned int irq,
> if (!fmb->mii_bus || fmb->mii_bus->state != MDIOBUS_REGISTERED)
> return ERR_PTR(-EPROBE_DEFER);
>
> - /* Check if we have a GPIO associated with this fixed phy */
> - gpiod = fixed_phy_get_gpiod(np);
> - if (IS_ERR(gpiod))
> - return ERR_CAST(gpiod);
> -
> /* Get the next available PHY address, up to PHY_MAX_ADDR */
> phy_addr = ida_simple_get(&phy_fixed_ida, 0, PHY_MAX_ADDR, GFP_KERNEL);
> if (phy_addr < 0)
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 3d92ea6fcc02..bd88f0aef2fa 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2051,7 +2051,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
> phydev = phy_find_first(dev->mdiobus);
> if (!phydev) {
> netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
> - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
> + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
> if (IS_ERR(phydev)) {
> netdev_err(dev->net, "No PHY/fixed_PHY found\n");
> return NULL;
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index de6157357e26..6be2120b5f03 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -20,6 +20,7 @@
> #include <linux/of_mdio.h>
> #include <linux/of_net.h>
> #include <linux/module.h>
> +#include <linux/gpio/consumer.h>
>
> #define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
>
> @@ -460,6 +461,7 @@ int of_phy_register_fixed_link(struct device_node *np)
> {
> struct fixed_phy_status status = {};
> struct device_node *fixed_link_node;
> + struct gpio_desc *gpiod = NULL;
> u32 fixed_link_prop[5];
> const char *managed;
>
> @@ -483,7 +485,17 @@ int of_phy_register_fixed_link(struct device_node *np)
> status.pause = of_property_read_bool(fixed_link_node, "pause");
> status.asym_pause = of_property_read_bool(fixed_link_node,
> "asym-pause");
> +
> + gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0,
> + GPIOD_IN, "mdio");
> of_node_put(fixed_link_node);
> + if (IS_ERR(gpiod)) {
> + if (PTR_ERR(gpiod) == -EPROBE_DEFER)
> + return PTR_ERR(gpiod);
> + pr_err("error getting GPIO for fixed link %pOF, proceed without\n",
> + fixed_link_node);
> + gpiod = NULL;
> extern struct phy_device *fixed_phy_register(unsigned int irq,
> struct fixed_phy_status *status,
> - struct device_node *np);
> + struct device_node *np,
> + struct gpio_desc *gpiod);
Hi Moritz
I think it would be better to add a
extern struct phy_device *fixed_phy_register_gpiod(unsigned int irq,
struct fixed_phy_status *status,
struct gpio_desc *gpiod);
If you are not using DT, the np is pointless. So lets keep the API
simple.
Andrew
^ permalink raw reply
* Re: [PATCH net-next v4 00/12] net: Introduce ndo_get_port_parent_id()
From: David Miller @ 2019-02-06 21:50 UTC (permalink / raw)
To: f.fainelli
Cc: netdev, idosch, linux-kernel, linux-rdma, oss-drivers, devel,
bridge
In-Reply-To: <20190206174546.23597-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 6 Feb 2019 09:45:34 -0800
> Based on discussion with Ido and feedback from Jakub there are clearly
> two classes of users that implement SWITCHDEV_ATTR_ID_PORT_PARENT_ID:
>
> - PF/VF drivers which typically only implement return the port's parent
> ID, yet have to implement switchdev_port_attr_get() just for that
>
> - Ethernet switch drivers: mlxsw, ocelot, DSA, etc. which implement more
> attributes which we want to be able to eventually veto in the context
> of the caller, thus making them candidates for using a blocking notifier
> chain
>
> Changes in v4:
..
Series applied, thanks Florian.
I'll push this out to net-next after my build tests complete.
^ permalink raw reply
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
From: Julien Gomes @ 2019-02-06 21:48 UTC (permalink / raw)
To: Neil Horman, Marcelo Ricardo Leitner
Cc: netdev, linux-sctp, linux-kernel, davem, vyasevich, lucien.xin
In-Reply-To: <20190206212316.GD16887@hmswarspite.think-freely.org>
On 2/6/19 1:23 PM, Neil Horman wrote:
> On Wed, Feb 06, 2019 at 07:07:23PM -0200, Marcelo Ricardo Leitner wrote:
>> On Wed, Feb 06, 2019 at 12:48:38PM -0800, Julien Gomes wrote:
>>>
>>>
>>> On 2/6/19 12:37 PM, Marcelo Ricardo Leitner wrote:
>>>> On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
>>>>> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
>>>>> structures longer than the current definitions.
>>>>>
>>>>> This should prevent unjustified setsockopt() failures due to struct
>>>>> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
>>>>> binaries that should be compatible, but were built with later kernel
>>>>> uapi headers.
>>>>
>>>> Not sure if we support backwards compatibility like this?
>>>>
>>>> My issue with this change is that by doing this, application will have
>>>> no clue if the new bits were ignored or not and it may think that an
>>>> event is enabled while it is not.
>>>>
>>>> A workaround would be to do a getsockopt and check the size that was
>>>> returned. But then, it might as well use the right struct here in the
>>>> first place.
>>>>
>>>> I'm seeing current implementation as an implicitly versioned argument:
>>>> it will always accept setsockopt calls with an old struct (v4.11 or
>>>> v4.12), but if the user tries to use v3 on a v1-only system, it will
>>>> be rejected. Pretty much like using a newer setsockopt on an old
>>>> system.
>>>
>>> With the current implementation, given sources that say are supposed to
>>> run on a 4.9 kernel (no use of any newer field added in 4.11 or 4.12),
>>> we can't rebuild the exact same sources on a 4.19 kernel and still run
>>> them on 4.9 without messing with structures re-definition.
>>
>> Maybe what we want(ed) here then is explicit versioning, to have the 3
>> definitions available. Then the application is able to use, say struct
>> sctp_event_subscribe, and be happy with it, while there is struct
>> sctp_event_subscribe_v2 and struct sctp_event_subscribe_v3 there too.
>>
>> But it's too late for that now because that would break applications
>> already using the new fields in sctp_event_subscribe.
>>
> Yeah, I'm not supportive of codifying that knoweldge in the kernel. If we were
> to support bi-directional versioning, I would encode it into lksctp-tools rather
> than the kernel.
I'm not sure that forcing a library on users is a good reason to break UAPI.
>
>>>
>>> I understand your point, but this still looks like a sort of uapi
>>> breakage to me.
>>
>> Not disagreeing. I really just don't know how supported that is.
>> Willing to know so I can pay more attention to this on future changes.
>>
>> Btw, is this the only occurrence?
>>
> No, I think there are a few others (maybe paddrparams?)
>
>>>
>>>
>>> I also had another way to work-around this in mind, by copying optlen
>>> bytes and checking that any additional field (not included in the
>>> "current" kernel structure definition) is not set, returning EINVAL in
>>> such case to keep a similar to current behavior.
>>> The issue with this is that I didn't find a suitable (ie not totally
>>> arbitrary such as "twice the existing structure size") upper limit to
>>> optlen.
>>
>> Seems interesting. Why would it need that upper limit to optlen?
>>
> I think the thought was to differentiate between someone passing a legit larger
> structure from a newer UAPI, from someone just passing in a massive
> inappropriately sized buffer (even if the return on both is the same).
>
>> Say struct v1 had 4 bytes, v3 now had 12. The user supplies 12 bytes
>> to the kernel that only knows about 4 bytes. It can check that (12-4)
>> bytes in the end, make sure no bit is on and use only the first 4.
>>
>> The fact that it was 12 or 200 shouldn't matter, should it? As long as
>> the (200-4) bytes are 0'ed, only the first 4 will be used and it
>> should be ok, otherwise EINVAL. No need to know how big the current
>> current actually is because it wouldn't be validating that here: just
>> that it can safely use the first 4 bytes.
>>
> I'm less than excited about making the kernel check an unbounded user space
> buffer, thats seems like a potential DOS attack from an unpriviledged user to
> me. I'm also still hung up on the notion that, despite how we do this, this
> patch is going into the latest kernel, so it will only work on a kernel that
> already understands the most recent set of subscriptions. It would work if we,
> again someday in the future extended this struct, someone built against that
> newer UAPI, and then tried to run it on a kernel that had this patch.
The patch is going into the latest, but can also be backported on future
stables.
I don't think "not fixing it because it's not fixed yet" is a good
reason to keep things the way they are. But maybe that's just me.
Given that the structure has already been extended several times, there
is pretty much nothing to keep this from happening again and again.
>
> FWIW, there is an existing implied method to determine the available
> subscription events. sctp_getsockopt_events does clamp the size of the output
> buffer, and returns that information in the optlen field via put_user. An
> application that was build against UAPIs from 4.19 could pass in the 4.19
> sctp_event_subscribe struct to sctp_getsockopt_events, and read the output
> length, whcih would inform the application of the events that the kernel is
> capable of reporting, and limit itself to only using those events. Its not a
> perfect solution, but its direct, understandable and portable.
>
> Neil
>
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Julien Gomes <julien@arista.com>
>>>>> ---
>>>>> net/sctp/socket.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>>> index 9644bdc8e85c..f9717e2789da 100644
>>>>> --- a/net/sctp/socket.c
>>>>> +++ b/net/sctp/socket.c
>>>>> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
>>>>> int i;
>>>>>
>>>>> if (optlen > sizeof(struct sctp_event_subscribe))
>>>>> - return -EINVAL;
>>>>> + optlen = sizeof(struct sctp_event_subscribe);
>>>>>
>>>>> if (copy_from_user(&subscribe, optval, optlen))
>>>>> return -EFAULT;
>>>>> --
>>>>> 2.20.1
>>>>>
>>>
>>
--
Julien Gomes
^ permalink raw reply
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
From: Julien Gomes @ 2019-02-06 21:48 UTC (permalink / raw)
To: Neil Horman
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
In-Reply-To: <20190206213948.GE16887@hmswarspite.think-freely.org>
On 2/6/19 1:39 PM, Neil Horman wrote:
> On Wed, Feb 06, 2019 at 01:26:55PM -0800, Julien Gomes wrote:
>>
>>
>> On 2/6/19 1:07 PM, Marcelo Ricardo Leitner wrote:
>>> On Wed, Feb 06, 2019 at 12:48:38PM -0800, Julien Gomes wrote:
>>>>
>>>>
>>>> On 2/6/19 12:37 PM, Marcelo Ricardo Leitner wrote:
>>>>> On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
>>>>>> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
>>>>>> structures longer than the current definitions.
>>>>>>
>>>>>> This should prevent unjustified setsockopt() failures due to struct
>>>>>> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
>>>>>> binaries that should be compatible, but were built with later kernel
>>>>>> uapi headers.
>>>>>
>>>>> Not sure if we support backwards compatibility like this?
>>>>>
>>>>> My issue with this change is that by doing this, application will have
>>>>> no clue if the new bits were ignored or not and it may think that an
>>>>> event is enabled while it is not.
>>>>>
>>>>> A workaround would be to do a getsockopt and check the size that was
>>>>> returned. But then, it might as well use the right struct here in the
>>>>> first place.
>>>>>
>>>>> I'm seeing current implementation as an implicitly versioned argument:
>>>>> it will always accept setsockopt calls with an old struct (v4.11 or
>>>>> v4.12), but if the user tries to use v3 on a v1-only system, it will
>>>>> be rejected. Pretty much like using a newer setsockopt on an old
>>>>> system.
>>>>
>>>> With the current implementation, given sources that say are supposed to
>>>> run on a 4.9 kernel (no use of any newer field added in 4.11 or 4.12),
>>>> we can't rebuild the exact same sources on a 4.19 kernel and still run
>>>> them on 4.9 without messing with structures re-definition.
>>>
>>> Maybe what we want(ed) here then is explicit versioning, to have the 3
>>> definitions available. Then the application is able to use, say struct
>>> sctp_event_subscribe, and be happy with it, while there is struct
>>> sctp_event_subscribe_v2 and struct sctp_event_subscribe_v3 there too.
>>>
>>> But it's too late for that now because that would break applications
>>> already using the new fields in sctp_event_subscribe.
>>
>> Right.
>>
>>>
>>>>
>>>> I understand your point, but this still looks like a sort of uapi
>>>> breakage to me.
>>>
>>> Not disagreeing. I really just don't know how supported that is.
>>> Willing to know so I can pay more attention to this on future changes.
>>>
>>> Btw, is this the only occurrence?
>>
>> Can't really say, this is one I witnessed, I haven't really looked for
>> others.
>>
>>>
>>>>
>>>>
>>>> I also had another way to work-around this in mind, by copying optlen
>>>> bytes and checking that any additional field (not included in the
>>>> "current" kernel structure definition) is not set, returning EINVAL in
>>>> such case to keep a similar to current behavior.
>>>> The issue with this is that I didn't find a suitable (ie not totally
>>>> arbitrary such as "twice the existing structure size") upper limit to
>>>> optlen.
>>>
>>> Seems interesting. Why would it need that upper limit to optlen?
>>>
>>> Say struct v1 had 4 bytes, v3 now had 12. The user supplies 12 bytes
>>> to the kernel that only knows about 4 bytes. It can check that (12-4)
>>> bytes in the end, make sure no bit is on and use only the first 4.
>>>
>>> The fact that it was 12 or 200 shouldn't matter, should it? As long as
>>> the (200-4) bytes are 0'ed, only the first 4 will be used and it
>>> should be ok, otherwise EINVAL. No need to know how big the current
>>> current actually is because it wouldn't be validating that here: just
>>> that it can safely use the first 4 bytes.
>>
>> The upper limit concern is more regarding the call to copy_from_user
>> with an unrestricted/unchecked value.
> Copy_from_user should be safe to copy an arbitrary amount, the only restriction
> is that optlen can't exceed the size of the buffer receiving the data in the
> kernel. From that standpoint your patch is safe. However, that exposes the
> problem of checking any tail data on the userspace buffer. That is to say, if
> you want to ensure that any extra data that gets sent from userspace isn't
> 'set', you would have to copy that extra data in consumable chunks and check
> them individaully, and that screams DOS to me (i.e. imagine a user passing in a
> 4GB buffer, and having to wait for the kernel to copy each X sized chunk,
> looking for non-zero values).
There probably is a decent compromise to find between "not accepting a
single additional byte" and accepting several GB.
For example how likely is it that the growth of this structure make it
go over a page? I would hope not at all.
By choosing a large but decent high limit, I think we can find a
future-compatible compromise that doesn't rely on a preliminary
getsockopt() just for structure trucation decision...
>
>> I am not sure of how much of a risk/how exploitable this could be,
>> that's why I cautiously wanted to limit it in the first place just in case.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Julien Gomes <julien@arista.com>
>>>>>> ---
>>>>>> net/sctp/socket.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>>>> index 9644bdc8e85c..f9717e2789da 100644
>>>>>> --- a/net/sctp/socket.c
>>>>>> +++ b/net/sctp/socket.c
>>>>>> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
>>>>>> int i;
>>>>>>
>>>>>> if (optlen > sizeof(struct sctp_event_subscribe))
>>>>>> - return -EINVAL;
>>>>>> + optlen = sizeof(struct sctp_event_subscribe);
>>>>>>
>>>>>> if (copy_from_user(&subscribe, optval, optlen))
>>>>>> return -EFAULT;
>>>>>> --
>>>>>> 2.20.1
>>>>>>
>>>>
>>
>> --
>> Julien Gomes
>>
--
Julien Gomes
^ permalink raw reply
* Re: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes
From: Andrew Lunn @ 2019-02-06 21:44 UTC (permalink / raw)
To: Leo Li
Cc: Pankaj Bansal, Shawn Guo, Florian Fainelli,
netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org
In-Reply-To: <AM6PR04MB58636BDAAAAEA71B7B1DC7F28F6F0@AM6PR04MB5863.eurprd04.prod.outlook.com>
> > > > &i2c0 {
> > > > status = "okay";
> > > >
> > > > + fpga@66 {
> > > > + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c";
> > > > + reg = <0x66>;
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > > > +
> > > > + mdio-mux-1@54 {
> > >
> > > Still no compatible string defined for the node. Probably should be
> > > "mdio-mux- mmioreg", "mdio-mux"
> >
> > it is not a specific device. MDIO mux is meant to be controlled by some
> > registers of parent device (FPGA).
> > Therefore, IMO this should not be a device and there should not be any
> > "compatible" property for it.
> If it is not a device why we are defining a device node for it? It
> is probably not a physical device per se, but it can be considered a
> virtual device provided by FPGA.
It is a physical device. But it happens to be embedded inside another
device. And that embedded is not performed as a bus with devices on
it, so the device tree concepts don't fit directly.
> This also bring up another question that why this device cannot
> reuse the existing drivers/net/phy/mdio-mux-mmioreg.c driver?
Because it is on an i2c bus, not an mmio bus.
> If we think regmap is a better solution, shall we replace the
> mmioreg driver with the regmap driver?
regmap can be used with mmio. But for a single MMIO register it is a
huge framework. So it makes sense to keep mdio-mux-mmioreg simple.
If however the device is already using regmap, adding one more
register is very little overhead. And it might be possible to use this
new mux with an mmio regmap, or an spi regmap, etc. So we seem to be
covering the best of both worlds.
Andrew
^ permalink raw reply
* Re: [PATCH net-next] cxgb4: Update 1.22.9.0 as the latest firmware supported.
From: David Miller @ 2019-02-06 21:43 UTC (permalink / raw)
To: vishal; +Cc: netdev, nirranjan, indranil, dt
In-Reply-To: <1549458096-17880-1-git-send-email-vishal@chelsio.com>
From: Vishal Kulkarni <vishal@chelsio.com>
Date: Wed, 6 Feb 2019 18:31:36 +0530
> Change t4fw_version.h to update latest firmware version
> number to 1.22.9.0.
>
> Signed-off-by: Vishal Kulkarni <vishal@chelsio.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] cxgb4: Add new T6 PCI device ids 0x608b
From: David Miller @ 2019-02-06 21:43 UTC (permalink / raw)
To: vishal; +Cc: netdev, nirranjan, indranil, dt
In-Reply-To: <1549457833-17602-1-git-send-email-vishal@chelsio.com>
From: Vishal Kulkarni <vishal@chelsio.com>
Date: Wed, 6 Feb 2019 18:27:13 +0530
> Signed-off-by: Vishal Kulkarni <vishal@chelsio.com>
Applied.
^ permalink raw reply
* Re: [PATCH v3 2/2] r8169: Avoid pointer aliasing
From: David Miller @ 2019-02-06 21:40 UTC (permalink / raw)
To: thierry.reding
Cc: hkallweit1, andrew, joe, eric.dumazet, pauldzim, mkubecek,
nic_swsd, netdev, linux-kernel
In-Reply-To: <20190206123018.24802-2-thierry.reding@gmail.com>
From: Thierry Reding <thierry.reding@gmail.com>
Date: Wed, 6 Feb 2019 13:30:18 +0100
> From: Thierry Reding <treding@nvidia.com>
>
> Read MAC address 32-bit at a time and manually extract the individual
> bytes. This avoids pointer aliasing and gives the compiler a better
> chance of optimizing the operation.
>
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
Applied.
^ permalink raw reply
* Re: [PATCH v3 1/2] r8169: Load MAC address from device tree if present
From: David Miller @ 2019-02-06 21:40 UTC (permalink / raw)
To: thierry.reding
Cc: hkallweit1, andrew, joe, eric.dumazet, pauldzim, mkubecek,
nic_swsd, netdev, linux-kernel
In-Reply-To: <20190206123018.24802-1-thierry.reding@gmail.com>
From: Thierry Reding <thierry.reding@gmail.com>
Date: Wed, 6 Feb 2019 13:30:17 +0100
> From: Thierry Reding <treding@nvidia.com>
>
> If the system was booted using a device tree and if the device tree
> contains a MAC address, use it instead of reading one from the EEPROM.
> This is useful in situations where the EEPROM isn't properly programmed
> or where the firmware wants to override the existing MAC address.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
Applied.
^ permalink raw reply
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
From: Neil Horman @ 2019-02-06 21:39 UTC (permalink / raw)
To: Julien Gomes
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
In-Reply-To: <274c48cb-7590-682b-f4de-f5c4ce2d2144@arista.com>
On Wed, Feb 06, 2019 at 01:26:55PM -0800, Julien Gomes wrote:
>
>
> On 2/6/19 1:07 PM, Marcelo Ricardo Leitner wrote:
> > On Wed, Feb 06, 2019 at 12:48:38PM -0800, Julien Gomes wrote:
> >>
> >>
> >> On 2/6/19 12:37 PM, Marcelo Ricardo Leitner wrote:
> >>> On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
> >>>> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
> >>>> structures longer than the current definitions.
> >>>>
> >>>> This should prevent unjustified setsockopt() failures due to struct
> >>>> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
> >>>> binaries that should be compatible, but were built with later kernel
> >>>> uapi headers.
> >>>
> >>> Not sure if we support backwards compatibility like this?
> >>>
> >>> My issue with this change is that by doing this, application will have
> >>> no clue if the new bits were ignored or not and it may think that an
> >>> event is enabled while it is not.
> >>>
> >>> A workaround would be to do a getsockopt and check the size that was
> >>> returned. But then, it might as well use the right struct here in the
> >>> first place.
> >>>
> >>> I'm seeing current implementation as an implicitly versioned argument:
> >>> it will always accept setsockopt calls with an old struct (v4.11 or
> >>> v4.12), but if the user tries to use v3 on a v1-only system, it will
> >>> be rejected. Pretty much like using a newer setsockopt on an old
> >>> system.
> >>
> >> With the current implementation, given sources that say are supposed to
> >> run on a 4.9 kernel (no use of any newer field added in 4.11 or 4.12),
> >> we can't rebuild the exact same sources on a 4.19 kernel and still run
> >> them on 4.9 without messing with structures re-definition.
> >
> > Maybe what we want(ed) here then is explicit versioning, to have the 3
> > definitions available. Then the application is able to use, say struct
> > sctp_event_subscribe, and be happy with it, while there is struct
> > sctp_event_subscribe_v2 and struct sctp_event_subscribe_v3 there too.
> >
> > But it's too late for that now because that would break applications
> > already using the new fields in sctp_event_subscribe.
>
> Right.
>
> >
> >>
> >> I understand your point, but this still looks like a sort of uapi
> >> breakage to me.
> >
> > Not disagreeing. I really just don't know how supported that is.
> > Willing to know so I can pay more attention to this on future changes.
> >
> > Btw, is this the only occurrence?
>
> Can't really say, this is one I witnessed, I haven't really looked for
> others.
>
> >
> >>
> >>
> >> I also had another way to work-around this in mind, by copying optlen
> >> bytes and checking that any additional field (not included in the
> >> "current" kernel structure definition) is not set, returning EINVAL in
> >> such case to keep a similar to current behavior.
> >> The issue with this is that I didn't find a suitable (ie not totally
> >> arbitrary such as "twice the existing structure size") upper limit to
> >> optlen.
> >
> > Seems interesting. Why would it need that upper limit to optlen?
> >
> > Say struct v1 had 4 bytes, v3 now had 12. The user supplies 12 bytes
> > to the kernel that only knows about 4 bytes. It can check that (12-4)
> > bytes in the end, make sure no bit is on and use only the first 4.
> >
> > The fact that it was 12 or 200 shouldn't matter, should it? As long as
> > the (200-4) bytes are 0'ed, only the first 4 will be used and it
> > should be ok, otherwise EINVAL. No need to know how big the current
> > current actually is because it wouldn't be validating that here: just
> > that it can safely use the first 4 bytes.
>
> The upper limit concern is more regarding the call to copy_from_user
> with an unrestricted/unchecked value.
Copy_from_user should be safe to copy an arbitrary amount, the only restriction
is that optlen can't exceed the size of the buffer receiving the data in the
kernel. From that standpoint your patch is safe. However, that exposes the
problem of checking any tail data on the userspace buffer. That is to say, if
you want to ensure that any extra data that gets sent from userspace isn't
'set', you would have to copy that extra data in consumable chunks and check
them individaully, and that screams DOS to me (i.e. imagine a user passing in a
4GB buffer, and having to wait for the kernel to copy each X sized chunk,
looking for non-zero values).
> I am not sure of how much of a risk/how exploitable this could be,
> that's why I cautiously wanted to limit it in the first place just in case.
>
> >
> >>
> >>>
> >>>>
> >>>> Signed-off-by: Julien Gomes <julien@arista.com>
> >>>> ---
> >>>> net/sctp/socket.c | 2 +-
> >>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> >>>> index 9644bdc8e85c..f9717e2789da 100644
> >>>> --- a/net/sctp/socket.c
> >>>> +++ b/net/sctp/socket.c
> >>>> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
> >>>> int i;
> >>>>
> >>>> if (optlen > sizeof(struct sctp_event_subscribe))
> >>>> - return -EINVAL;
> >>>> + optlen = sizeof(struct sctp_event_subscribe);
> >>>>
> >>>> if (copy_from_user(&subscribe, optval, optlen))
> >>>> return -EFAULT;
> >>>> --
> >>>> 2.20.1
> >>>>
> >>
>
> --
> Julien Gomes
>
^ permalink raw reply
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
From: Julien Gomes @ 2019-02-06 21:26 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: netdev, linux-sctp, linux-kernel, davem, nhorman, vyasevich,
lucien.xin
In-Reply-To: <20190206210723.GD13621@localhost.localdomain>
On 2/6/19 1:07 PM, Marcelo Ricardo Leitner wrote:
> On Wed, Feb 06, 2019 at 12:48:38PM -0800, Julien Gomes wrote:
>>
>>
>> On 2/6/19 12:37 PM, Marcelo Ricardo Leitner wrote:
>>> On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
>>>> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
>>>> structures longer than the current definitions.
>>>>
>>>> This should prevent unjustified setsockopt() failures due to struct
>>>> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
>>>> binaries that should be compatible, but were built with later kernel
>>>> uapi headers.
>>>
>>> Not sure if we support backwards compatibility like this?
>>>
>>> My issue with this change is that by doing this, application will have
>>> no clue if the new bits were ignored or not and it may think that an
>>> event is enabled while it is not.
>>>
>>> A workaround would be to do a getsockopt and check the size that was
>>> returned. But then, it might as well use the right struct here in the
>>> first place.
>>>
>>> I'm seeing current implementation as an implicitly versioned argument:
>>> it will always accept setsockopt calls with an old struct (v4.11 or
>>> v4.12), but if the user tries to use v3 on a v1-only system, it will
>>> be rejected. Pretty much like using a newer setsockopt on an old
>>> system.
>>
>> With the current implementation, given sources that say are supposed to
>> run on a 4.9 kernel (no use of any newer field added in 4.11 or 4.12),
>> we can't rebuild the exact same sources on a 4.19 kernel and still run
>> them on 4.9 without messing with structures re-definition.
>
> Maybe what we want(ed) here then is explicit versioning, to have the 3
> definitions available. Then the application is able to use, say struct
> sctp_event_subscribe, and be happy with it, while there is struct
> sctp_event_subscribe_v2 and struct sctp_event_subscribe_v3 there too.
>
> But it's too late for that now because that would break applications
> already using the new fields in sctp_event_subscribe.
Right.
>
>>
>> I understand your point, but this still looks like a sort of uapi
>> breakage to me.
>
> Not disagreeing. I really just don't know how supported that is.
> Willing to know so I can pay more attention to this on future changes.
>
> Btw, is this the only occurrence?
Can't really say, this is one I witnessed, I haven't really looked for
others.
>
>>
>>
>> I also had another way to work-around this in mind, by copying optlen
>> bytes and checking that any additional field (not included in the
>> "current" kernel structure definition) is not set, returning EINVAL in
>> such case to keep a similar to current behavior.
>> The issue with this is that I didn't find a suitable (ie not totally
>> arbitrary such as "twice the existing structure size") upper limit to
>> optlen.
>
> Seems interesting. Why would it need that upper limit to optlen?
>
> Say struct v1 had 4 bytes, v3 now had 12. The user supplies 12 bytes
> to the kernel that only knows about 4 bytes. It can check that (12-4)
> bytes in the end, make sure no bit is on and use only the first 4.
>
> The fact that it was 12 or 200 shouldn't matter, should it? As long as
> the (200-4) bytes are 0'ed, only the first 4 will be used and it
> should be ok, otherwise EINVAL. No need to know how big the current
> current actually is because it wouldn't be validating that here: just
> that it can safely use the first 4 bytes.
The upper limit concern is more regarding the call to copy_from_user
with an unrestricted/unchecked value.
I am not sure of how much of a risk/how exploitable this could be,
that's why I cautiously wanted to limit it in the first place just in case.
>
>>
>>>
>>>>
>>>> Signed-off-by: Julien Gomes <julien@arista.com>
>>>> ---
>>>> net/sctp/socket.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>> index 9644bdc8e85c..f9717e2789da 100644
>>>> --- a/net/sctp/socket.c
>>>> +++ b/net/sctp/socket.c
>>>> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
>>>> int i;
>>>>
>>>> if (optlen > sizeof(struct sctp_event_subscribe))
>>>> - return -EINVAL;
>>>> + optlen = sizeof(struct sctp_event_subscribe);
>>>>
>>>> if (copy_from_user(&subscribe, optval, optlen))
>>>> return -EFAULT;
>>>> --
>>>> 2.20.1
>>>>
>>
--
Julien Gomes
^ permalink raw reply
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
From: Neil Horman @ 2019-02-06 21:23 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Julien Gomes, netdev, linux-sctp, linux-kernel, davem, vyasevich,
lucien.xin
In-Reply-To: <20190206210723.GD13621@localhost.localdomain>
On Wed, Feb 06, 2019 at 07:07:23PM -0200, Marcelo Ricardo Leitner wrote:
> On Wed, Feb 06, 2019 at 12:48:38PM -0800, Julien Gomes wrote:
> >
> >
> > On 2/6/19 12:37 PM, Marcelo Ricardo Leitner wrote:
> > > On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
> > >> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
> > >> structures longer than the current definitions.
> > >>
> > >> This should prevent unjustified setsockopt() failures due to struct
> > >> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
> > >> binaries that should be compatible, but were built with later kernel
> > >> uapi headers.
> > >
> > > Not sure if we support backwards compatibility like this?
> > >
> > > My issue with this change is that by doing this, application will have
> > > no clue if the new bits were ignored or not and it may think that an
> > > event is enabled while it is not.
> > >
> > > A workaround would be to do a getsockopt and check the size that was
> > > returned. But then, it might as well use the right struct here in the
> > > first place.
> > >
> > > I'm seeing current implementation as an implicitly versioned argument:
> > > it will always accept setsockopt calls with an old struct (v4.11 or
> > > v4.12), but if the user tries to use v3 on a v1-only system, it will
> > > be rejected. Pretty much like using a newer setsockopt on an old
> > > system.
> >
> > With the current implementation, given sources that say are supposed to
> > run on a 4.9 kernel (no use of any newer field added in 4.11 or 4.12),
> > we can't rebuild the exact same sources on a 4.19 kernel and still run
> > them on 4.9 without messing with structures re-definition.
>
> Maybe what we want(ed) here then is explicit versioning, to have the 3
> definitions available. Then the application is able to use, say struct
> sctp_event_subscribe, and be happy with it, while there is struct
> sctp_event_subscribe_v2 and struct sctp_event_subscribe_v3 there too.
>
> But it's too late for that now because that would break applications
> already using the new fields in sctp_event_subscribe.
>
Yeah, I'm not supportive of codifying that knoweldge in the kernel. If we were
to support bi-directional versioning, I would encode it into lksctp-tools rather
than the kernel.
> >
> > I understand your point, but this still looks like a sort of uapi
> > breakage to me.
>
> Not disagreeing. I really just don't know how supported that is.
> Willing to know so I can pay more attention to this on future changes.
>
> Btw, is this the only occurrence?
>
No, I think there are a few others (maybe paddrparams?)
> >
> >
> > I also had another way to work-around this in mind, by copying optlen
> > bytes and checking that any additional field (not included in the
> > "current" kernel structure definition) is not set, returning EINVAL in
> > such case to keep a similar to current behavior.
> > The issue with this is that I didn't find a suitable (ie not totally
> > arbitrary such as "twice the existing structure size") upper limit to
> > optlen.
>
> Seems interesting. Why would it need that upper limit to optlen?
>
I think the thought was to differentiate between someone passing a legit larger
structure from a newer UAPI, from someone just passing in a massive
inappropriately sized buffer (even if the return on both is the same).
> Say struct v1 had 4 bytes, v3 now had 12. The user supplies 12 bytes
> to the kernel that only knows about 4 bytes. It can check that (12-4)
> bytes in the end, make sure no bit is on and use only the first 4.
>
> The fact that it was 12 or 200 shouldn't matter, should it? As long as
> the (200-4) bytes are 0'ed, only the first 4 will be used and it
> should be ok, otherwise EINVAL. No need to know how big the current
> current actually is because it wouldn't be validating that here: just
> that it can safely use the first 4 bytes.
>
I'm less than excited about making the kernel check an unbounded user space
buffer, thats seems like a potential DOS attack from an unpriviledged user to
me. I'm also still hung up on the notion that, despite how we do this, this
patch is going into the latest kernel, so it will only work on a kernel that
already understands the most recent set of subscriptions. It would work if we,
again someday in the future extended this struct, someone built against that
newer UAPI, and then tried to run it on a kernel that had this patch.
FWIW, there is an existing implied method to determine the available
subscription events. sctp_getsockopt_events does clamp the size of the output
buffer, and returns that information in the optlen field via put_user. An
application that was build against UAPIs from 4.19 could pass in the 4.19
sctp_event_subscribe struct to sctp_getsockopt_events, and read the output
length, whcih would inform the application of the events that the kernel is
capable of reporting, and limit itself to only using those events. Its not a
perfect solution, but its direct, understandable and portable.
Neil
> >
> > >
> > >>
> > >> Signed-off-by: Julien Gomes <julien@arista.com>
> > >> ---
> > >> net/sctp/socket.c | 2 +-
> > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > >> index 9644bdc8e85c..f9717e2789da 100644
> > >> --- a/net/sctp/socket.c
> > >> +++ b/net/sctp/socket.c
> > >> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
> > >> int i;
> > >>
> > >> if (optlen > sizeof(struct sctp_event_subscribe))
> > >> - return -EINVAL;
> > >> + optlen = sizeof(struct sctp_event_subscribe);
> > >>
> > >> if (copy_from_user(&subscribe, optval, optlen))
> > >> return -EFAULT;
> > >> --
> > >> 2.20.1
> > >>
> >
>
^ permalink raw reply
* Re: stmmac / meson8b-dwmac
From: Simon Huelck @ 2019-02-06 21:21 UTC (permalink / raw)
To: Emiliano Ingrassia, Martin Blumenstingl
Cc: Gpeppe.cavallaro, alexandre.torgue, linux-amlogic, netdev
In-Reply-To: <20190206103645.GA2482@ingrassia.epigenesys.com>
Hi,
4.17.9 failed going back further
regards,
Simon
Am 06.02.2019 um 11:36 schrieb Emiliano Ingrassia:
> Hi Martin, Hi Simon,
>
> On Mon, Feb 04, 2019 at 03:34:41PM +0100, Martin Blumenstingl wrote:
>> On Thu, Jan 17, 2019 at 10:23 PM Simon Huelck <simonmail@gmx.de> wrote:
>> [...]
>>>>> I got problems with my ODROID c2 running on 4.19.16 ( and some releases
>>>>> earlier ). the stmmac / dwmac driver doesnt provide the 800M/900M
>>>>> performance that i was used to earlier.
>>>>>
> Simon, did you ever reach 1 Gbps full duplex speed?
> If yes, what was the kernel version did you use?
>
>>>>> Now im stuck near 550M/600M in the same environment. but what really
>>>>> confuses me that duplex does hurt even more.
>>>> interesting that you see this on the Odroid-C2 as well.
>>>> previously I have only observed it on an Odroid-C1
>>>>
>>>>> PC --- VLAN3 --> switch --VLAN3--> ODROID
>>>>>
>>>>> NAS <-- VLAN1 -- switch <-- VLAN1-- ODROID
>>>>>
>>>>>
>>>>> this means when im doing a iperf from PC to NAS, that my ODROID has load
>>>>> on RX/TX same time (duplex). this shouldnt be an issue , all is 1GBits
>>>>> FD. And in the past that wasnt an issue.
>> +Cc Emiliano who has seen a similar duplex issue on his Odroid-C1: [0]
>> (please note that all kernels prior to v5.1 with the pending patches
>> from [1] applied are only receiving data on RXD0 and RXD1 but not on
>> RXD2 and RXD3)
>>
>> Emiliano, can you confirm the duplex issue observed by Simon is
>> similar to the one you see on your Odroid-C1?
>>
> It could be but, if I understand correctly, Simon is limited in
> speed also in half duplex transmission (~550/600 Mbps), while we can
> reach at least 900 Mbps.
>
>>>>>
>>>>> Now what happens:
>>>>>
>>>>> - benchmark between PC - ODROID is roughly 550M
>>>>>
>>>>> - benchmark between NAS - ODROID is roughly 550M
>>>>>
>>>>> - benchmark between PC - NAS is only around 300M
>>>>>
>>>>>
>>>>> and like i said i was easliy able to hit 800 or even 900M to my NAS
>>>>> earlier. I applied some .dtb fixes for interrupt levels for the
>>>>> meson-gx.dtsi and meson-gxbb-odroid-c2.dtb, which will be mainlined ,
>>>>> but the effect stayed identical.
>>>> good that you have the interrupt patches already applied
>>>> I believe it don't fix any performance issues - it's a fix for the
>>>> Ethernet controller seemingly getting "stuck" (not processing data
>>>> anymore). however, that already rules out one potential issue
>>>>
>>>>> are you aware of this problem ? Earlier kernel versions were all
>>>>> perfectly fine and i stepped ( self compiled) kernel through all major
>>>>> releases since odroid c2 was mainlined.
>> Guiseppe, Alexandre: what kind of data do you need from us if we see
>> the speeds drop (in both directions) when we send and receive at the
>> same time?
>>
>> [...]
>>> the problem is that i dont have these kernel sources anymore :-(. but i
>>> can provide some testing and numbers. maybe i dig if i got these kernel
>>> configs somewhere around but i did not change much during migrating
>> do you remember the kernel version where it worked fine?
>>
>>> im using a zyxel gs1900-8 switch and a qnap ts231p , and as i said i
>>> didnt change my setup. i was able to hit 100MByte/s from my NAS , so
>>> close to the benchmarks of 900MBit/s
>> I typically only do small transfers or I have traffic only in one direction.
>> thus it's likely that I missed this in my own tests
>>
>>
>> Regards
>> Martin
>>
>>
>> [0] http://lists.infradead.org/pipermail/linux-amlogic/2018-December/009679.html
>> [1] https://patchwork.kernel.org/cover/10744905/
> Regards,
>
> Emiliano
^ permalink raw reply
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
From: Marcelo Ricardo Leitner @ 2019-02-06 21:18 UTC (permalink / raw)
To: Neil Horman
Cc: Julien Gomes, netdev, linux-sctp, linux-kernel, davem, vyasevich,
lucien.xin
In-Reply-To: <20190206210827.GC16887@hmswarspite.think-freely.org>
On Wed, Feb 06, 2019 at 04:08:27PM -0500, Neil Horman wrote:
> On Wed, Feb 06, 2019 at 12:48:38PM -0800, Julien Gomes wrote:
> >
> >
> > On 2/6/19 12:37 PM, Marcelo Ricardo Leitner wrote:
> > > On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
> > >> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
> > >> structures longer than the current definitions.
> > >>
> > >> This should prevent unjustified setsockopt() failures due to struct
> > >> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
> > >> binaries that should be compatible, but were built with later kernel
> > >> uapi headers.
> > >
> > > Not sure if we support backwards compatibility like this?
> > >
> > > My issue with this change is that by doing this, application will have
> > > no clue if the new bits were ignored or not and it may think that an
> > > event is enabled while it is not.
> > >
> > > A workaround would be to do a getsockopt and check the size that was
> > > returned. But then, it might as well use the right struct here in the
> > > first place.
> > >
> > > I'm seeing current implementation as an implicitly versioned argument:
> > > it will always accept setsockopt calls with an old struct (v4.11 or
> > > v4.12), but if the user tries to use v3 on a v1-only system, it will
> > > be rejected. Pretty much like using a newer setsockopt on an old
> > > system.
> >
> > With the current implementation, given sources that say are supposed to
> > run on a 4.9 kernel (no use of any newer field added in 4.11 or 4.12),
> What given sources say that? I understand it might be expected, but this is an
> common concern with setsockopt method on many protocols, it just so happens that
> sctp extends them more than other protocols.
>
> > we can't rebuild the exact same sources on a 4.19 kernel and still run
> > them on 4.9 without messing with structures re-definition.
> >
> Right, put another way, we support backward compatibility with older userspace
> applications, but not newer one. I.e. if you build an application against the
> 4.9 SCTP API, it should work with the 4.19 UAPI, but not vice versa, which it
> seems is like what you are trying to do here.
Was looking for that. Thanks.
>
> > I understand your point, but this still looks like a sort of uapi
> > breakage to me.
> >
> >
> > I also had another way to work-around this in mind, by copying optlen
> > bytes and checking that any additional field (not included in the
> > "current" kernel structure definition) is not set, returning EINVAL in
> > such case to keep a similar to current behavior.
> > The issue with this is that I didn't find a suitable (ie not totally
> > arbitrary such as "twice the existing structure size") upper limit to
> > optlen.
> >
> There is no real uppper limit to the size of the structure in this case, and
> IIRC this isn't the only sockopt structure that can be exentded for SCTP in this
> way.
>
> I really don't see a sane way to allow newer userspaces to be compatible with
> older kernels here. If we were to do it I would suggest moving the
> responsibility for that feature into lksctp-tools, versioning that library such
> that correlary symbols are versioned to translate the application view of the
> socket options structs to the size and format that the running kernel
> undertands. Note that I'm not really advocating for that, as it seems like a
> fast moving target, but if we were to do it I think that would be the most sane
> way to handle it.
Speaking of that, recent lksctp-tools got some defines to help knowing
which features the available kernel headers have as it now probes if
specific struct members are available or not. Though yeah, it also
wouldn't help in this case, just mentioning it.
>
> Neil
>
> > >
> > >>
> > >> Signed-off-by: Julien Gomes <julien@arista.com>
> > >> ---
> > >> net/sctp/socket.c | 2 +-
> > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > >> index 9644bdc8e85c..f9717e2789da 100644
> > >> --- a/net/sctp/socket.c
> > >> +++ b/net/sctp/socket.c
> > >> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
> > >> int i;
> > >>
> > >> if (optlen > sizeof(struct sctp_event_subscribe))
> > >> - return -EINVAL;
> > >> + optlen = sizeof(struct sctp_event_subscribe);
> > >>
> > >> if (copy_from_user(&subscribe, optval, optlen))
> > >> return -EFAULT;
> > >> --
> > >> 2.20.1
> > >>
> >
> >
^ permalink raw reply
* RE: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes
From: Leo Li @ 2019-02-06 21:17 UTC (permalink / raw)
To: Pankaj Bansal
Cc: Shawn Guo, Andrew Lunn, Florian Fainelli, netdev@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <VI1PR0401MB24968690F3E3677E5EE8DE49F16F0@VI1PR0401MB2496.eurprd04.prod.outlook.com>
> -----Original Message-----
> From: Pankaj Bansal
> Sent: Tuesday, February 5, 2019 10:02 PM
> To: Leo Li <leoyang.li@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>; Andrew Lunn <andrew@lunn.ch>;
> Florian Fainelli <f.fainelli@gmail.com>; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: RE: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes
>
>
>
> > -----Original Message-----
> > From: Li Yang [mailto:leoyang.li@nxp.com]
> > Sent: Wednesday, 6 February, 2019 12:07 AM
> > To: Pankaj Bansal <pankaj.bansal@nxp.com>
> > Cc: Shawn Guo <shawnguo@kernel.org>; Andrew Lunn
> <andrew@lunn.ch>;
> > Florian Fainelli <f.fainelli@gmail.com>; netdev@vger.kernel.org;
> > linux-arm- kernel@lists.infradead.org
> > Subject: Re: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes
> >
> > On Mon, Feb 4, 2019 at 2:53 AM Pankaj Bansal <pankaj.bansal@nxp.com>
> > wrote:
> > >
> > > The two external MDIO buses used to communicate with phy devices
> > > that are external to SOC are muxed in LX2160AQDS board.
> > >
> > > These buses can be routed to any one of the eight IO slots on
> > > LX2160AQDS board depending on value in fpga register 0x54.
> > >
> > > Additionally the external MDIO1 is used to communicate to the
> > > onboard RGMII phy devices.
> > >
> > > The mdio1 is controlled by bits 4-7 of fpga register and mdio2 is
> > > controlled by bits 0-3 of fpga register.
> > >
> > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > ---
> > >
> > > Notes:
> > > V2:
> > > - removed unnecassary TODO statements
> > > - removed device_type from mdio nodes
> > > - change the case of hex number to lowercase
> > > - removed board specific comments from soc file
> >
> > There are still some comments from V1 haven't been addressed.
> >
> > >
> > > .../boot/dts/freescale/fsl-lx2160a-qds.dts | 115 +++++++++++++++++
> > > .../boot/dts/freescale/fsl-lx2160a.dtsi | 18 +++
> > > 2 files changed, 133 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts
> > > b/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts
> > > index 99a22abbe725..2c3020a72d41 100644
> > > --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts
> > > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts
> > > @@ -46,6 +46,121 @@
> > > &i2c0 {
> > > status = "okay";
> > >
> > > + fpga@66 {
> > > + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c";
> > > + reg = <0x66>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + mdio-mux-1@54 {
> >
> > Still no compatible string defined for the node. Probably should be
> > "mdio-mux- mmioreg", "mdio-mux"
>
> it is not a specific device. MDIO mux is meant to be controlled by some
> registers of parent device (FPGA).
> Therefore, IMO this should not be a device and there should not be any
> "compatible" property for it.
If it is not a device why we are defining a device node for it? It is probably not a physical device per se, but it can be considered a virtual device provided by FPGA.
This also bring up another question that why this device cannot reuse the existing drivers/net/phy/mdio-mux-mmioreg.c driver? If we think regmap is a better solution, shall we replace the mmioreg driver with the regmap driver?
>
> >
> > > + mdio-parent-bus = <&emdio1>;
> > > + reg = <0x54>; /* BRDCFG4 */
> > > + mux-mask = <0xf8>; /* EMI1_MDIO */
> > > + #address-cells=<1>;
> > > + #size-cells = <0>;
> > > +
> > > + mdio@0 {
> > > + reg = <0x00>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@40 {
> > > + reg = <0x40>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@c0 {
> > > + reg = <0xc0>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@c8 {
> > > + reg = <0xc8>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@d0 {
> > > + reg = <0xd0>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@d8 {
> > > + reg = <0xd8>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@e0 {
> > > + reg = <0xe0>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@e8 {
> > > + reg = <0xe8>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@f0 {
> > > + reg = <0xf0>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@f8 {
> > > + reg = <0xf8>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + };
> > > +
> > > + mdio-mux-2@54 {
> > > + mdio-parent-bus = <&emdio2>;
> > > + reg = <0x54>; /* BRDCFG4 */
> > > + mux-mask = <0x07>; /* EMI2_MDIO */
> > > + #address-cells=<1>;
> > > + #size-cells = <0>;
> > > +
> > > + mdio@0 {
> > > + reg = <0x00>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@1 {
> > > + reg = <0x01>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@2 {
> > > + reg = <0x02>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@3 {
> > > + reg = <0x03>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@4 {
> > > + reg = <0x04>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@5 {
> > > + reg = <0x05>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@6 {
> > > + reg = <0x06>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + mdio@7 {
> > > + reg = <0x07>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > > + };
> > > + };
> > > +
> > > i2c-mux@77 {
> > > compatible = "nxp,pca9547";
> > > reg = <0x77>;
> > > diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
> > > b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
> > > index a79f5c1ea56d..a74045ad22ad 100644
> > > --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
> > > @@ -762,5 +762,23 @@
> > > <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>;
> > > dma-coherent;
> > > };
> > > +
> > > + /* WRIOP0: 0x8b8_0000, E-MDIO1: 0x1_6000 */
> > > + emdio1: mdio@8b96000 {
> > > + compatible = "fsl,fman-memac-mdio";
> > > + reg = <0x0 0x8b96000 0x0 0x1000>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + little-endian; /* force the driver in LE
> > > + mode */
> >
> > Still doesn't fully align with the binding at
> > "Documentation/devicetree/bindings/powerpc/fsl/fman.txt".
> >
> > It should either have the "interrupts" property for external MDIO or
> > "fsl,fman- internal-mdio" for internal MDIO.
>
> I see that for DPAA1 based SOCs, there was definitely an interrupt property
> in external MDIO node.
> BUT, for DPAA2 based devices none of the SOC has this property. I am
> looking further into this.
>
> >
> > > + };
> > > +
> > > + /* WRIOP0: 0x8b8_0000, E-MDIO2: 0x1_7000 */
> > > + emdio2: mdio@8b97000 {
> > > + compatible = "fsl,fman-memac-mdio";
> > > + reg = <0x0 0x8b97000 0x0 0x1000>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + little-endian; /* force the driver in LE mode */
> > > + };
> > > };
> > > };
> > > --
> > > 2.17.1
> > >
^ permalink raw reply
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
From: Neil Horman @ 2019-02-06 21:08 UTC (permalink / raw)
To: Julien Gomes
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
In-Reply-To: <c13067c0-f127-50a0-9ae0-4be7f0399c0d@arista.com>
On Wed, Feb 06, 2019 at 12:48:38PM -0800, Julien Gomes wrote:
>
>
> On 2/6/19 12:37 PM, Marcelo Ricardo Leitner wrote:
> > On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
> >> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
> >> structures longer than the current definitions.
> >>
> >> This should prevent unjustified setsockopt() failures due to struct
> >> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
> >> binaries that should be compatible, but were built with later kernel
> >> uapi headers.
> >
> > Not sure if we support backwards compatibility like this?
> >
> > My issue with this change is that by doing this, application will have
> > no clue if the new bits were ignored or not and it may think that an
> > event is enabled while it is not.
> >
> > A workaround would be to do a getsockopt and check the size that was
> > returned. But then, it might as well use the right struct here in the
> > first place.
> >
> > I'm seeing current implementation as an implicitly versioned argument:
> > it will always accept setsockopt calls with an old struct (v4.11 or
> > v4.12), but if the user tries to use v3 on a v1-only system, it will
> > be rejected. Pretty much like using a newer setsockopt on an old
> > system.
>
> With the current implementation, given sources that say are supposed to
> run on a 4.9 kernel (no use of any newer field added in 4.11 or 4.12),
What given sources say that? I understand it might be expected, but this is an
common concern with setsockopt method on many protocols, it just so happens that
sctp extends them more than other protocols.
> we can't rebuild the exact same sources on a 4.19 kernel and still run
> them on 4.9 without messing with structures re-definition.
>
Right, put another way, we support backward compatibility with older userspace
applications, but not newer one. I.e. if you build an application against the
4.9 SCTP API, it should work with the 4.19 UAPI, but not vice versa, which it
seems is like what you are trying to do here.
> I understand your point, but this still looks like a sort of uapi
> breakage to me.
>
>
> I also had another way to work-around this in mind, by copying optlen
> bytes and checking that any additional field (not included in the
> "current" kernel structure definition) is not set, returning EINVAL in
> such case to keep a similar to current behavior.
> The issue with this is that I didn't find a suitable (ie not totally
> arbitrary such as "twice the existing structure size") upper limit to
> optlen.
>
There is no real uppper limit to the size of the structure in this case, and
IIRC this isn't the only sockopt structure that can be exentded for SCTP in this
way.
I really don't see a sane way to allow newer userspaces to be compatible with
older kernels here. If we were to do it I would suggest moving the
responsibility for that feature into lksctp-tools, versioning that library such
that correlary symbols are versioned to translate the application view of the
socket options structs to the size and format that the running kernel
undertands. Note that I'm not really advocating for that, as it seems like a
fast moving target, but if we were to do it I think that would be the most sane
way to handle it.
Neil
> >
> >>
> >> Signed-off-by: Julien Gomes <julien@arista.com>
> >> ---
> >> net/sctp/socket.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> >> index 9644bdc8e85c..f9717e2789da 100644
> >> --- a/net/sctp/socket.c
> >> +++ b/net/sctp/socket.c
> >> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
> >> int i;
> >>
> >> if (optlen > sizeof(struct sctp_event_subscribe))
> >> - return -EINVAL;
> >> + optlen = sizeof(struct sctp_event_subscribe);
> >>
> >> if (copy_from_user(&subscribe, optval, optlen))
> >> return -EFAULT;
> >> --
> >> 2.20.1
> >>
>
>
^ permalink raw reply
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
From: Marcelo Ricardo Leitner @ 2019-02-06 21:07 UTC (permalink / raw)
To: Julien Gomes
Cc: netdev, linux-sctp, linux-kernel, davem, nhorman, vyasevich,
lucien.xin
In-Reply-To: <c13067c0-f127-50a0-9ae0-4be7f0399c0d@arista.com>
On Wed, Feb 06, 2019 at 12:48:38PM -0800, Julien Gomes wrote:
>
>
> On 2/6/19 12:37 PM, Marcelo Ricardo Leitner wrote:
> > On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
> >> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
> >> structures longer than the current definitions.
> >>
> >> This should prevent unjustified setsockopt() failures due to struct
> >> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
> >> binaries that should be compatible, but were built with later kernel
> >> uapi headers.
> >
> > Not sure if we support backwards compatibility like this?
> >
> > My issue with this change is that by doing this, application will have
> > no clue if the new bits were ignored or not and it may think that an
> > event is enabled while it is not.
> >
> > A workaround would be to do a getsockopt and check the size that was
> > returned. But then, it might as well use the right struct here in the
> > first place.
> >
> > I'm seeing current implementation as an implicitly versioned argument:
> > it will always accept setsockopt calls with an old struct (v4.11 or
> > v4.12), but if the user tries to use v3 on a v1-only system, it will
> > be rejected. Pretty much like using a newer setsockopt on an old
> > system.
>
> With the current implementation, given sources that say are supposed to
> run on a 4.9 kernel (no use of any newer field added in 4.11 or 4.12),
> we can't rebuild the exact same sources on a 4.19 kernel and still run
> them on 4.9 without messing with structures re-definition.
Maybe what we want(ed) here then is explicit versioning, to have the 3
definitions available. Then the application is able to use, say struct
sctp_event_subscribe, and be happy with it, while there is struct
sctp_event_subscribe_v2 and struct sctp_event_subscribe_v3 there too.
But it's too late for that now because that would break applications
already using the new fields in sctp_event_subscribe.
>
> I understand your point, but this still looks like a sort of uapi
> breakage to me.
Not disagreeing. I really just don't know how supported that is.
Willing to know so I can pay more attention to this on future changes.
Btw, is this the only occurrence?
>
>
> I also had another way to work-around this in mind, by copying optlen
> bytes and checking that any additional field (not included in the
> "current" kernel structure definition) is not set, returning EINVAL in
> such case to keep a similar to current behavior.
> The issue with this is that I didn't find a suitable (ie not totally
> arbitrary such as "twice the existing structure size") upper limit to
> optlen.
Seems interesting. Why would it need that upper limit to optlen?
Say struct v1 had 4 bytes, v3 now had 12. The user supplies 12 bytes
to the kernel that only knows about 4 bytes. It can check that (12-4)
bytes in the end, make sure no bit is on and use only the first 4.
The fact that it was 12 or 200 shouldn't matter, should it? As long as
the (200-4) bytes are 0'ed, only the first 4 will be used and it
should be ok, otherwise EINVAL. No need to know how big the current
current actually is because it wouldn't be validating that here: just
that it can safely use the first 4 bytes.
>
> >
> >>
> >> Signed-off-by: Julien Gomes <julien@arista.com>
> >> ---
> >> net/sctp/socket.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> >> index 9644bdc8e85c..f9717e2789da 100644
> >> --- a/net/sctp/socket.c
> >> +++ b/net/sctp/socket.c
> >> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
> >> int i;
> >>
> >> if (optlen > sizeof(struct sctp_event_subscribe))
> >> - return -EINVAL;
> >> + optlen = sizeof(struct sctp_event_subscribe);
> >>
> >> if (copy_from_user(&subscribe, optval, optlen))
> >> return -EFAULT;
> >> --
> >> 2.20.1
> >>
>
^ permalink raw reply
* Re: [PATCH net] rxrpc: bad unlock balance in rxrpc_recvmsg
From: David Howells @ 2019-02-06 21:05 UTC (permalink / raw)
To: Eric Dumazet; +Cc: dhowells, David S . Miller, netdev, Eric Dumazet, syzbot
In-Reply-To: <20190204163606.236419-1-edumazet@google.com>
Eric Dumazet <edumazet@google.com> wrote:
> When either "goto wait_interrupted;" or "goto wait_error;"
> paths are taken, socket lock has already been released.
>
> This patch fixes following syzbot splat :
>
> WARNING: bad unlock balance detected!
> 5.0.0-rc4+ #59 Not tainted
> -------------------------------------
> syz-executor223/8256 is trying to release lock (sk_lock-AF_RXRPC) at:
> [<ffffffff86651353>] rxrpc_recvmsg+0x6d3/0x3099 net/rxrpc/recvmsg.c:598
> but there are no more locks to release!
>
> other info that might help us debug this:
> 1 lock held by syz-executor223/8256:
> #0: 00000000fa9ed0f4 (slock-AF_RXRPC){+...}, at: spin_lock_bh include/linux/spinlock.h:334 [inline]
> #0: 00000000fa9ed0f4 (slock-AF_RXRPC){+...}, at: release_sock+0x20/0x1c0 net/core/sock.c:2798
>
> stack backtrace:
> CPU: 1 PID: 8256 Comm: syz-executor223 Not tainted 5.0.0-rc4+ #59
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x172/0x1f0 lib/dump_stack.c:113
> print_unlock_imbalance_bug kernel/locking/lockdep.c:3391 [inline]
> print_unlock_imbalance_bug.cold+0x114/0x123 kernel/locking/lockdep.c:3368
> __lock_release kernel/locking/lockdep.c:3601 [inline]
> lock_release+0x67e/0xa00 kernel/locking/lockdep.c:3860
> sock_release_ownership include/net/sock.h:1471 [inline]
> release_sock+0x183/0x1c0 net/core/sock.c:2808
> rxrpc_recvmsg+0x6d3/0x3099 net/rxrpc/recvmsg.c:598
> sock_recvmsg_nosec net/socket.c:794 [inline]
> sock_recvmsg net/socket.c:801 [inline]
> sock_recvmsg+0xd0/0x110 net/socket.c:797
> __sys_recvfrom+0x1ff/0x350 net/socket.c:1845
> __do_sys_recvfrom net/socket.c:1863 [inline]
> __se_sys_recvfrom net/socket.c:1859 [inline]
> __x64_sys_recvfrom+0xe1/0x1a0 net/socket.c:1859
> do_syscall_64+0x103/0x610 arch/x86/entry/common.c:290
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x446379
> Code: e8 2c b3 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 2b 09 fc ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007fe5da89fd98 EFLAGS: 00000246 ORIG_RAX: 000000000000002d
> RAX: ffffffffffffffda RBX: 00000000006dbc28 RCX: 0000000000446379
> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003
> RBP: 00000000006dbc20 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00000000006dbc2c
> R13: 0000000000000000 R14: 0000000000000000 R15: 20c49ba5e353f7cf
>
> Fixes: 248f219cb8bc ("rxrpc: Rewrite the data and ack handling code")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: David Howells <dhowells@redhat.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
Acked-by: David Howells <dhowells@redhat.com>
^ permalink raw reply
* [RFC net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c
From: Moritz Fischer @ 2019-02-06 20:51 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, netdev, colin.king, linus.walleij, yuehaibing, mcgrof,
frowand.list, robh+dt, UNGLinuxDriver, woojung.huh, hkallweit1,
davem, f.fainelli, vivien.didelot, andrew, moritz, alex.williams,
Moritz Fischer
Move the DT based link GPIO parsing to of_mdio and let the places
that register a fixed_phy pass in a GPIO descriptor or NULL.
This allows fixed_phy on non-DT platforms to have link GPIOs, too.
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
drivers/net/dsa/dsa_loop.c | 2 +-
drivers/net/ethernet/broadcom/bgmac.c | 2 +-
drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 +-
drivers/net/phy/fixed_phy.c | 48 ++------------------
drivers/net/usb/lan78xx.c | 2 +-
drivers/of/of_mdio.c | 15 +++++-
include/linux/phy_fixed.h | 3 +-
7 files changed, 23 insertions(+), 51 deletions(-)
diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
index 17482ae09aa5..7f124c620092 100644
--- a/drivers/net/dsa/dsa_loop.c
+++ b/drivers/net/dsa/dsa_loop.c
@@ -343,7 +343,7 @@ static int __init dsa_loop_init(void)
unsigned int i;
for (i = 0; i < NUM_FIXED_PHYS; i++)
- phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL);
+ phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL, NULL);
return mdio_driver_register(&dsa_loop_drv);
}
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 4632dd5dbad1..bce644dec5c2 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1446,7 +1446,7 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac)
struct phy_device *phy_dev;
int err;
- phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
+ phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
if (!phy_dev || IS_ERR(phy_dev)) {
dev_err(bgmac->dev, "Failed to register fixed PHY device\n");
return -ENODEV;
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 51880d83131a..7cbd737aba80 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -525,7 +525,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
.asym_pause = 0,
};
- phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
+ phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
if (!phydev || IS_ERR(phydev)) {
dev_err(kdev, "failed to register fixed PHY device\n");
return -ENODEV;
diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index d810f914aaa4..845bd7c2065a 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -18,6 +18,7 @@
#include <linux/err.h>
#include <linux/slab.h>
#include <linux/of.h>
+#include <linux/of_mdio.h>
#include <linux/gpio/consumer.h>
#include <linux/seqlock.h>
#include <linux/idr.h>
@@ -191,50 +192,12 @@ static void fixed_phy_del(int phy_addr)
}
}
-#ifdef CONFIG_OF_GPIO
-static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np)
-{
- struct device_node *fixed_link_node;
- struct gpio_desc *gpiod;
-
- if (!np)
- return NULL;
-
- fixed_link_node = of_get_child_by_name(np, "fixed-link");
- if (!fixed_link_node)
- return NULL;
-
- /*
- * As the fixed link is just a device tree node without any
- * Linux device associated with it, we simply have obtain
- * the GPIO descriptor from the device tree like this.
- */
- gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0,
- GPIOD_IN, "mdio");
- of_node_put(fixed_link_node);
- if (IS_ERR(gpiod)) {
- if (PTR_ERR(gpiod) == -EPROBE_DEFER)
- return gpiod;
- pr_err("error getting GPIO for fixed link %pOF, proceed without\n",
- fixed_link_node);
- gpiod = NULL;
- }
-
- return gpiod;
-}
-#else
-static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np)
-{
- return NULL;
-}
-#endif
-
struct phy_device *fixed_phy_register(unsigned int irq,
struct fixed_phy_status *status,
- struct device_node *np)
+ struct device_node *np,
+ struct gpio_desc *gpiod)
{
struct fixed_mdio_bus *fmb = &platform_fmb;
- struct gpio_desc *gpiod = NULL;
struct phy_device *phy;
int phy_addr;
int ret;
@@ -242,11 +205,6 @@ struct phy_device *fixed_phy_register(unsigned int irq,
if (!fmb->mii_bus || fmb->mii_bus->state != MDIOBUS_REGISTERED)
return ERR_PTR(-EPROBE_DEFER);
- /* Check if we have a GPIO associated with this fixed phy */
- gpiod = fixed_phy_get_gpiod(np);
- if (IS_ERR(gpiod))
- return ERR_CAST(gpiod);
-
/* Get the next available PHY address, up to PHY_MAX_ADDR */
phy_addr = ida_simple_get(&phy_fixed_ida, 0, PHY_MAX_ADDR, GFP_KERNEL);
if (phy_addr < 0)
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 3d92ea6fcc02..bd88f0aef2fa 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2051,7 +2051,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
phydev = phy_find_first(dev->mdiobus);
if (!phydev) {
netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
- phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
+ phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
if (IS_ERR(phydev)) {
netdev_err(dev->net, "No PHY/fixed_PHY found\n");
return NULL;
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index de6157357e26..6be2120b5f03 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -20,6 +20,7 @@
#include <linux/of_mdio.h>
#include <linux/of_net.h>
#include <linux/module.h>
+#include <linux/gpio/consumer.h>
#define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
@@ -460,6 +461,7 @@ int of_phy_register_fixed_link(struct device_node *np)
{
struct fixed_phy_status status = {};
struct device_node *fixed_link_node;
+ struct gpio_desc *gpiod = NULL;
u32 fixed_link_prop[5];
const char *managed;
@@ -483,7 +485,17 @@ int of_phy_register_fixed_link(struct device_node *np)
status.pause = of_property_read_bool(fixed_link_node, "pause");
status.asym_pause = of_property_read_bool(fixed_link_node,
"asym-pause");
+
+ gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0,
+ GPIOD_IN, "mdio");
of_node_put(fixed_link_node);
+ if (IS_ERR(gpiod)) {
+ if (PTR_ERR(gpiod) == -EPROBE_DEFER)
+ return PTR_ERR(gpiod);
+ pr_err("error getting GPIO for fixed link %pOF, proceed without\n",
+ fixed_link_node);
+ gpiod = NULL;
+ }
goto register_phy;
}
@@ -502,7 +514,8 @@ int of_phy_register_fixed_link(struct device_node *np)
return -ENODEV;
register_phy:
- return PTR_ERR_OR_ZERO(fixed_phy_register(PHY_POLL, &status, np));
+ return PTR_ERR_OR_ZERO(fixed_phy_register(PHY_POLL, &status, np,
+ gpiod));
}
EXPORT_SYMBOL(of_phy_register_fixed_link);
diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
index c78fc203db43..69caa8cda767 100644
--- a/include/linux/phy_fixed.h
+++ b/include/linux/phy_fixed.h
@@ -18,7 +18,8 @@ extern int fixed_phy_add(unsigned int irq, int phy_id,
struct fixed_phy_status *status);
extern struct phy_device *fixed_phy_register(unsigned int irq,
struct fixed_phy_status *status,
- struct device_node *np);
+ struct device_node *np,
+ struct gpio_desc *gpiod);
extern void fixed_phy_unregister(struct phy_device *phydev);
extern int fixed_phy_set_link_update(struct phy_device *phydev,
int (*link_update)(struct net_device *,
--
2.20.1
^ permalink raw reply related
* general protection fault in team_nl_cmd_options_set
From: syzbot @ 2019-02-06 21:03 UTC (permalink / raw)
To: davem, jiri, linux-kernel, netdev, syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: 9097a058d49e Merge branch 'i2c/for-current' of git://git.k..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11995667400000
kernel config: https://syzkaller.appspot.com/x/.config?x=861a3573f4e78ba1
dashboard link: https://syzkaller.appspot.com/bug?extid=68ee510075cf64260cc4
compiler: gcc (GCC) 8.0.1 20180413 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1263c10d400000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=130855b7400000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+68ee510075cf64260cc4@syzkaller.appspotmail.com
netlink: 'syz-executor043': attribute type 3 has an invalid length.
netlink: 'syz-executor043': attribute type 3 has an invalid length.
team0: Mode changed to "activebackup"
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 6431 Comm: syz-executor043 Not tainted 4.20.0-rc7+ #161
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
RIP: 0010:__team_option_inst_tmp_find drivers/net/team/team.c:264 [inline]
RIP: 0010:team_nl_cmd_options_set+0xa2d/0x1370 drivers/net/team/team.c:2591
Code: 8b 74 24 38 75 13 e9 eb 05 00 00 e8 4d 27 e0 fc 49 39 dc 0f 84 dd 05
00 00 e8 3f 27 e0 fc 49 8d 7c 24 10 48 89 f8 48 c1 e8 03 <42> 80 3c 38 00
0f 85 af 06 00 00 49 8b 44 24 10 4c 8d 60 f0 49 39
RSP: 0018:ffff8881af967270 EFLAGS: 00010203
RAX: 000000060dac2cae RBX: ffff8881d189dc00 RCX: ffffffff849f5cb7
RDX: 0000000000000000 RSI: ffffffff849f5d31 RDI: 000000306d616574
RBP: ffff8881af9674d0 R08: ffff8881cc646080 R09: ffffed103b5e5020
R10: ffffed103b5e5020 R11: ffff8881daf28107 R12: 000000306d616564
R13: ffff8881d189de10 R14: ffff8881af967360 R15: dffffc0000000000
FS: 00007fad37cf4700(0000) GS:ffff8881daf00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000004d1810 CR3: 00000001b96af000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
genl_family_rcv_msg+0x8a7/0x11a0 net/netlink/genetlink.c:601
genl_rcv_msg+0xc6/0x168 net/netlink/genetlink.c:626
netlink_rcv_skb+0x172/0x440 net/netlink/af_netlink.c:2477
genl_rcv+0x28/0x40 net/netlink/genetlink.c:637
netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
netlink_unicast+0x5a5/0x760 net/netlink/af_netlink.c:1336
netlink_sendmsg+0xa18/0xfc0 net/netlink/af_netlink.c:1917
sock_sendmsg_nosec net/socket.c:621 [inline]
sock_sendmsg+0xd5/0x120 net/socket.c:631
___sys_sendmsg+0x7fd/0x930 net/socket.c:2116
__sys_sendmsg+0x11d/0x280 net/socket.c:2154
__do_sys_sendmsg net/socket.c:2163 [inline]
__se_sys_sendmsg net/socket.c:2161 [inline]
__x64_sys_sendmsg+0x78/0xb0 net/socket.c:2161
do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x44de49
Code: e8 1c ba 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 fb c5 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fad37cf3d88 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00000000006e0c5c RCX: 000000000044de49
RDX: 0000000000000000 RSI: 0000000020000200 RDI: 0000000000000004
RBP: 00000000006e0c50 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000006c61767265
R13: 746e695f6e696f6a R14: 0000000000000000 R15: 0000000000000000
Modules linked in:
---[ end trace da19b5a20e6fec24 ]---
RIP: 0010:__team_option_inst_tmp_find drivers/net/team/team.c:264 [inline]
RIP: 0010:team_nl_cmd_options_set+0xa2d/0x1370 drivers/net/team/team.c:2591
Code: 8b 74 24 38 75 13 e9 eb 05 00 00 e8 4d 27 e0 fc 49 39 dc 0f 84 dd 05
00 00 e8 3f 27 e0 fc 49 8d 7c 24 10 48 89 f8 48 c1 e8 03 <42> 80 3c 38 00
0f 85 af 06 00 00 49 8b 44 24 10 4c 8d 60 f0 49 39
RSP: 0018:ffff8881af967270 EFLAGS: 00010203
RAX: 000000060dac2cae RBX: ffff8881d189dc00 RCX: ffffffff849f5cb7
RDX: 0000000000000000 RSI: ffffffff849f5d31 RDI: 000000306d616574
RBP: ffff8881af9674d0 R08: ffff8881cc646080 R09: ffffed103b5e5020
R10: ffffed103b5e5020 R11: ffff8881daf28107 R12: 000000306d616564
R13: ffff8881d189de10 R14: ffff8881af967360 R15: dffffc0000000000
FS: 00007fad37cf4700(0000) GS:ffff8881daf00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000004d1810 CR3: 00000001b96af000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
From: Neil Horman @ 2019-02-06 20:49 UTC (permalink / raw)
To: Julien Gomes
Cc: netdev, linux-sctp, linux-kernel, davem, marcelo.leitner,
vyasevich, lucien.xin
In-Reply-To: <20190206201430.18830-1-julien@arista.com>
On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
> structures longer than the current definitions.
>
> This should prevent unjustified setsockopt() failures due to struct
> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
> binaries that should be compatible, but were built with later kernel
> uapi headers.
>
> Signed-off-by: Julien Gomes <julien@arista.com>
> ---
> net/sctp/socket.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 9644bdc8e85c..f9717e2789da 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
> int i;
>
> if (optlen > sizeof(struct sctp_event_subscribe))
> - return -EINVAL;
> + optlen = sizeof(struct sctp_event_subscribe);
>
I'm not sure I like this. If you have a userspace application built against
more recent uapi headers than the kernel you are actually running on, then by
defintion you won't have this check in place, and you'll get EINVAL returns
anyway. If you just backport this patch to an older kernel, you'll not get the
EINVAL return, but you will get silent failures on event subscriptions that your
application thinks exists, but the kernel doesn't recognize.
This would make sense if you had a way to communicate back to user space the
unrecognized options, but since we don't (currently) have that, I would rather
see the EINVAL returned than just have things not work.
Neil
> if (copy_from_user(&subscribe, optval, optlen))
> return -EFAULT;
> --
> 2.20.1
>
>
^ permalink raw reply
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
From: Julien Gomes @ 2019-02-06 20:48 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: netdev, linux-sctp, linux-kernel, davem, nhorman, vyasevich,
lucien.xin
In-Reply-To: <20190206203754.GC13621@localhost.localdomain>
On 2/6/19 12:37 PM, Marcelo Ricardo Leitner wrote:
> On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
>> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
>> structures longer than the current definitions.
>>
>> This should prevent unjustified setsockopt() failures due to struct
>> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
>> binaries that should be compatible, but were built with later kernel
>> uapi headers.
>
> Not sure if we support backwards compatibility like this?
>
> My issue with this change is that by doing this, application will have
> no clue if the new bits were ignored or not and it may think that an
> event is enabled while it is not.
>
> A workaround would be to do a getsockopt and check the size that was
> returned. But then, it might as well use the right struct here in the
> first place.
>
> I'm seeing current implementation as an implicitly versioned argument:
> it will always accept setsockopt calls with an old struct (v4.11 or
> v4.12), but if the user tries to use v3 on a v1-only system, it will
> be rejected. Pretty much like using a newer setsockopt on an old
> system.
With the current implementation, given sources that say are supposed to
run on a 4.9 kernel (no use of any newer field added in 4.11 or 4.12),
we can't rebuild the exact same sources on a 4.19 kernel and still run
them on 4.9 without messing with structures re-definition.
I understand your point, but this still looks like a sort of uapi
breakage to me.
I also had another way to work-around this in mind, by copying optlen
bytes and checking that any additional field (not included in the
"current" kernel structure definition) is not set, returning EINVAL in
such case to keep a similar to current behavior.
The issue with this is that I didn't find a suitable (ie not totally
arbitrary such as "twice the existing structure size") upper limit to
optlen.
>
>>
>> Signed-off-by: Julien Gomes <julien@arista.com>
>> ---
>> net/sctp/socket.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 9644bdc8e85c..f9717e2789da 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
>> int i;
>>
>> if (optlen > sizeof(struct sctp_event_subscribe))
>> - return -EINVAL;
>> + optlen = sizeof(struct sctp_event_subscribe);
>>
>> if (copy_from_user(&subscribe, optval, optlen))
>> return -EFAULT;
>> --
>> 2.20.1
>>
^ permalink raw reply
* [PATCH net-next 4/4] net: dsa: bcm_sf2: Allow looping back CFP rules
From: Florian Fainelli @ 2019-02-06 20:46 UTC (permalink / raw)
To: netdev; +Cc: davem, cphealy, vivien.didelot, Florian Fainelli
In-Reply-To: <20190206204600.24109-1-f.fainelli@gmail.com>
When the source and destination port of a CFP rule match, we must set
the loopback bit enable to allow that, otherwise the frame is discarded.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/dsa/bcm_sf2_cfp.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/net/dsa/bcm_sf2_cfp.c b/drivers/net/dsa/bcm_sf2_cfp.c
index 8747d18297fa..0b9ca4bdf47e 100644
--- a/drivers/net/dsa/bcm_sf2_cfp.c
+++ b/drivers/net/dsa/bcm_sf2_cfp.c
@@ -213,6 +213,7 @@ static inline unsigned int bcm_sf2_cfp_rule_size(struct bcm_sf2_priv *priv)
static int bcm_sf2_cfp_act_pol_set(struct bcm_sf2_priv *priv,
unsigned int rule_index,
+ int src_port,
unsigned int port_num,
unsigned int queue_num,
bool fwd_map_change)
@@ -230,6 +231,10 @@ static int bcm_sf2_cfp_act_pol_set(struct bcm_sf2_priv *priv,
else
reg = 0;
+ /* Enable looping back to the original port */
+ if (src_port == port_num)
+ reg |= LOOP_BK_EN;
+
core_writel(priv, reg, CORE_ACT_POL_DATA0);
/* Set classification ID that needs to be put in Broadcom tag */
@@ -443,7 +448,7 @@ static int bcm_sf2_cfp_ipv4_rule_set(struct bcm_sf2_priv *priv, int port,
}
/* Insert into Action and policer RAMs now */
- ret = bcm_sf2_cfp_act_pol_set(priv, rule_index, port_num,
+ ret = bcm_sf2_cfp_act_pol_set(priv, rule_index, port, port_num,
queue_num, true);
if (ret)
goto out_err_flow_rule;
@@ -733,7 +738,7 @@ static int bcm_sf2_cfp_ipv6_rule_set(struct bcm_sf2_priv *priv, int port,
}
/* Insert into Action and policer RAMs now */
- ret = bcm_sf2_cfp_act_pol_set(priv, rule_index[0], port_num,
+ ret = bcm_sf2_cfp_act_pol_set(priv, rule_index[0], port, port_num,
queue_num, false);
if (ret)
goto out_err_flow_rule;
@@ -795,7 +800,7 @@ static int bcm_sf2_cfp_ipv6_rule_set(struct bcm_sf2_priv *priv, int port,
/* Insert into Action and policer RAMs now, set chain ID to
* the one we are chained to
*/
- ret = bcm_sf2_cfp_act_pol_set(priv, rule_index[1], port_num,
+ ret = bcm_sf2_cfp_act_pol_set(priv, rule_index[1], port, port_num,
queue_num, true);
if (ret)
goto out_err_flow_rule;
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 3/4] net: dsa: bcm_sf2: Add support for CFP statistics
From: Florian Fainelli @ 2019-02-06 20:45 UTC (permalink / raw)
To: netdev; +Cc: davem, cphealy, vivien.didelot, Florian Fainelli
In-Reply-To: <20190206204600.24109-1-f.fainelli@gmail.com>
Return CFP policer statistics (Green, Yellow or Red) as part of the
standard ethtool statistics. This helps debug when CFP rules may not be
hit (0 counter).
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/dsa/bcm_sf2.c | 16 ++++++-
drivers/net/dsa/bcm_sf2.h | 5 ++
drivers/net/dsa/bcm_sf2_cfp.c | 88 ++++++++++++++++++++++++++++++++++
drivers/net/dsa/bcm_sf2_regs.h | 4 ++
4 files changed, 112 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index ce5f4260ea1a..5193da67dcdc 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -897,19 +897,33 @@ static const struct b53_io_ops bcm_sf2_io_ops = {
static void bcm_sf2_sw_get_strings(struct dsa_switch *ds, int port,
u32 stringset, uint8_t *data)
{
+ int cnt = b53_get_sset_count(ds, port, stringset);
+
b53_get_strings(ds, port, stringset, data);
+ bcm_sf2_cfp_get_strings(ds, port, stringset,
+ data + cnt * ETH_GSTRING_LEN);
}
static void bcm_sf2_sw_get_ethtool_stats(struct dsa_switch *ds, int port,
uint64_t *data)
{
+ int cnt = b53_get_sset_count(ds, port, ETH_SS_STATS);
+
b53_get_ethtool_stats(ds, port, data);
+ bcm_sf2_cfp_get_ethtool_stats(ds, port, data + cnt);
}
static int bcm_sf2_sw_get_sset_count(struct dsa_switch *ds, int port,
int sset)
{
- return b53_get_sset_count(ds, port, sset);
+ int cnt = b53_get_sset_count(ds, port, sset);
+
+ if (cnt < 0)
+ return cnt;
+
+ cnt += bcm_sf2_cfp_get_sset_count(ds, port, sset);
+
+ return cnt;
}
static const struct dsa_switch_ops bcm_sf2_ops = {
diff --git a/drivers/net/dsa/bcm_sf2.h b/drivers/net/dsa/bcm_sf2.h
index 83e1d8001447..eb3655bea467 100644
--- a/drivers/net/dsa/bcm_sf2.h
+++ b/drivers/net/dsa/bcm_sf2.h
@@ -213,5 +213,10 @@ int bcm_sf2_set_rxnfc(struct dsa_switch *ds, int port,
int bcm_sf2_cfp_rst(struct bcm_sf2_priv *priv);
void bcm_sf2_cfp_exit(struct dsa_switch *ds);
int bcm_sf2_cfp_resume(struct dsa_switch *ds);
+void bcm_sf2_cfp_get_strings(struct dsa_switch *ds, int port,
+ u32 stringset, uint8_t *data);
+void bcm_sf2_cfp_get_ethtool_stats(struct dsa_switch *ds, int port,
+ uint64_t *data);
+int bcm_sf2_cfp_get_sset_count(struct dsa_switch *ds, int port, int sset);
#endif /* __BCM_SF2_H */
diff --git a/drivers/net/dsa/bcm_sf2_cfp.c b/drivers/net/dsa/bcm_sf2_cfp.c
index 6d8059dc77b7..8747d18297fa 100644
--- a/drivers/net/dsa/bcm_sf2_cfp.c
+++ b/drivers/net/dsa/bcm_sf2_cfp.c
@@ -1201,3 +1201,91 @@ int bcm_sf2_cfp_resume(struct dsa_switch *ds)
return ret;
}
+
+static const struct bcm_sf2_cfp_stat {
+ unsigned int offset;
+ unsigned int ram_loc;
+ const char *name;
+} bcm_sf2_cfp_stats[] = {
+ {
+ .offset = CORE_STAT_GREEN_CNTR,
+ .ram_loc = GREEN_STAT_RAM,
+ .name = "Green"
+ },
+ {
+ .offset = CORE_STAT_YELLOW_CNTR,
+ .ram_loc = YELLOW_STAT_RAM,
+ .name = "Yellow"
+ },
+ {
+ .offset = CORE_STAT_RED_CNTR,
+ .ram_loc = RED_STAT_RAM,
+ .name = "Red"
+ },
+};
+
+void bcm_sf2_cfp_get_strings(struct dsa_switch *ds, int port,
+ u32 stringset, uint8_t *data)
+{
+ struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
+ unsigned int s = ARRAY_SIZE(bcm_sf2_cfp_stats);
+ char buf[ETH_GSTRING_LEN];
+ unsigned int i, j, iter;
+
+ if (stringset != ETH_SS_STATS)
+ return;
+
+ for (i = 1; i < priv->num_cfp_rules; i++) {
+ for (j = 0; j < s; j++) {
+ snprintf(buf, sizeof(buf),
+ "CFP%03d_%sCntr",
+ i, bcm_sf2_cfp_stats[j].name);
+ iter = (i - 1) * s + j;
+ strlcpy(data + iter * ETH_GSTRING_LEN,
+ buf, ETH_GSTRING_LEN);
+ }
+ }
+}
+
+void bcm_sf2_cfp_get_ethtool_stats(struct dsa_switch *ds, int port,
+ uint64_t *data)
+{
+ struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
+ unsigned int s = ARRAY_SIZE(bcm_sf2_cfp_stats);
+ const struct bcm_sf2_cfp_stat *stat;
+ unsigned int i, j, iter;
+ struct cfp_rule *rule;
+ int ret;
+
+ mutex_lock(&priv->cfp.lock);
+ for (i = 1; i < priv->num_cfp_rules; i++) {
+ rule = bcm_sf2_cfp_rule_find(priv, port, i);
+ if (!rule)
+ continue;
+
+ for (j = 0; j < s; j++) {
+ stat = &bcm_sf2_cfp_stats[j];
+
+ bcm_sf2_cfp_rule_addr_set(priv, i);
+ ret = bcm_sf2_cfp_op(priv, stat->ram_loc | OP_SEL_READ);
+ if (ret)
+ continue;
+
+ iter = (i - 1) * s + j;
+ data[iter] = core_readl(priv, stat->offset);
+ }
+
+ }
+ mutex_unlock(&priv->cfp.lock);
+}
+
+int bcm_sf2_cfp_get_sset_count(struct dsa_switch *ds, int port, int sset)
+{
+ struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
+
+ if (sset != ETH_SS_STATS)
+ return 0;
+
+ /* 3 counters per CFP rules */
+ return (priv->num_cfp_rules - 1) * ARRAY_SIZE(bcm_sf2_cfp_stats);
+}
diff --git a/drivers/net/dsa/bcm_sf2_regs.h b/drivers/net/dsa/bcm_sf2_regs.h
index 0a1e530d52b7..67f056206f37 100644
--- a/drivers/net/dsa/bcm_sf2_regs.h
+++ b/drivers/net/dsa/bcm_sf2_regs.h
@@ -400,6 +400,10 @@ enum bcm_sf2_reg_offs {
#define CORE_RATE_METER6 0x281e0
#define CIR_REF_CNT_MASK 0x7ffff
+#define CORE_STAT_GREEN_CNTR 0x28200
+#define CORE_STAT_YELLOW_CNTR 0x28210
+#define CORE_STAT_RED_CNTR 0x28220
+
#define CORE_CFP_CTL_REG 0x28400
#define CFP_EN_MAP_MASK 0x1ff
--
2.17.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox