* Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver
From: Masahiro Yamada @ 2017-09-08 14:44 UTC (permalink / raw)
To: Kunihiko Hayashi
Cc: netdev, David S. Miller, Andrew Lunn, Florian Fainelli,
Rob Herring, Mark Rutland, linux-arm-kernel,
Linux Kernel Mailing List, devicetree, Masami Hiramatsu,
Jassi Brar
In-Reply-To: <1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>
2017-09-08 22:02 GMT+09:00 Kunihiko Hayashi <hayashi.kunihiko@socionext.com>:
> diff --git a/drivers/net/ethernet/socionext/Kconfig b/drivers/net/ethernet/socionext/Kconfig
> new file mode 100644
> index 0000000..788f26f
> --- /dev/null
> +++ b/drivers/net/ethernet/socionext/Kconfig
> @@ -0,0 +1,22 @@
> +config NET_VENDOR_SOCIONEXT
> + bool "Socionext ethernet drivers"
> + default y
> + ---help---
> + Option to select ethernet drivers for Socionext platforms.
> +
> + Note that the answer to this question doesn't directly affect the
> + kernel: saying N will just cause the configurator to skip all
> + the questions about Agere devices. If you say Y, you will be asked
> + for your specific card in the following questions.
Agere?
> +
> + dev_info(dev, "Socionext %c%c%c%c Ethernet IP %s (irq=%d, phy=%s)\n",
> + (ave_id >> 24) & 0xff, (ave_id >> 16) & 0xff,
> + (ave_id >> 8) & 0xff, (ave_id >> 0) & 0xff,
> + buf, ndev->irq, phy_modes(phy_mode));
> +
> + return 0;
> +err_netdev_register:
Maybe, a bad label name.
for ex. out_del_napi or whatever.
Documentation/process/coding-style.rst says
"Choose label names which say what the goto does ..."
> + netif_napi_del(&priv->napi_rx);
> + netif_napi_del(&priv->napi_tx);
> + mdiobus_unregister(priv->mdio);
> +err_mdiobus_register:
> +err_mdiobus_alloc:
> +err_req_irq:
These three should be merged, for ex.
out_free_device.
I ran sparse for you.
Please take a look if it is worthwhile.
Seems endianess around mac_addr.
drivers/net/ethernet/socionext/sni_ave.c:423:15: warning: cast to
restricted __le32
drivers/net/ethernet/socionext/sni_ave.c:425:20: warning: cast to
restricted __le16
drivers/net/ethernet/socionext/sni_ave.c:1194:15: warning: cast to
restricted __le32
drivers/net/ethernet/socionext/sni_ave.c:1196:20: warning: cast to
restricted __le16
drivers/net/ethernet/socionext/sni_ave.c:1398:15: warning: cast to
restricted __le32
drivers/net/ethernet/socionext/sni_ave.c:1400:20: warning: cast to
restricted __le16
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is not enough
From: Phil Sutter @ 2017-09-08 14:51 UTC (permalink / raw)
To: Hangbin Liu; +Cc: netdev, Stephen Hemminger, Michal Kubecek
In-Reply-To: <20170908140131.GU5465@leo.usersys.redhat.com>
Hi,
On Fri, Sep 08, 2017 at 10:01:31PM +0800, Hangbin Liu wrote:
[...]
> > > diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> > > index be7ac86..37cfb5a 100644
> > > --- a/lib/libnetlink.c
> > > +++ b/lib/libnetlink.c
> > > @@ -402,6 +402,59 @@ static void rtnl_dump_error(const struct rtnl_handle *rth,
> > > }
> > > }
> > >
> > > +static int rtnl_recvmsg(int fd, struct msghdr *msg, char **buf)
> > > +{
> > > + struct iovec *iov;
> > > + int len = -1, buf_len = 32768;
> > > + char *buffer = *buf;
> >
> > Isn't it possible to make 'buffer' static instead of the two 'buf'
> > variables in rtnl_dump_filter_l() and __rtnl_talk()? Then we would have
> > only a single buffer which is shared between both functions instead of
> > two which are independently allocated.
>
> I was also thinking of this before. But in function ipaddrlabel_flush()
>
> if (rtnl_dump_filter(&rth, flush_addrlabel, NULL) < 0)
>
> It will cal rtnl_dump_filter_l() first via
> rtnl_dump_filter() -> rtnl_dump_filter_nc() -> rtnl_dump_filter_l().
>
> Then call rtnl_talk() later via call back
> a->filter(&nladdr, h, a->arg1) -> flush_addrlabel() -> rtnl_talk()
>
> So if we only use one static buffer in rtnl_recvmsg(). Then it will be written
> at lease twice.
Oh yes, in that case we really can't have a single buffer.
[...]
> > > + buf_len = len;
> >
> > For this to work you have to make buf_len static too, otherwise you will
> > unnecessarily reallocate the buffer. Oh, and that also requires the
> > single buffer (as pointed out above) because you will otherwise use a
> > common buf_len for both static buffers passed to this function.
>
> Since we have to use two static bufffers. So how about check like
>
> if (len > strlen(buffer))
I don't think that will work. We are reusing the buffer and it contains
binary data, so a NUL byte may appear anywhere. I fear you will have to
change rtnl_recvmsg() to accept a buflen parameter which callers have to
define statically together with the buffer pointer.
Regarding Michal's concern about reentrancy, maybe we should go into a
different direction and make rtnl_recvmsg() return a newly allocated
buffer which the caller has to free.
[...]
> > When retrying inside rtnl_recvmsg(), it won't return 0 anymore. I
> > believe the whole 'while (1)' loop could go away then.
> >
>
> Like Michal said, there may have multi netlink packets?
Ah yes, I missed that.
Thanks, Phil
^ permalink raw reply
* Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree
From: Vivien Didelot @ 2017-09-08 14:57 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <20170828191748.19492-3-vivien.didelot@savoirfairelinux.com>
Hi Greg,
You wrote:
> > Can I ask for a quick review of this patch as well? It's the one adding
> > the boilerplate for a single debugfs file, and I'm pretty sure it can be
> > reduced somehow.
>
> I don't see a patch here :(
Oops, you weren't originally in Cc. Please find the patch below.
> > Also more important, you will notice what seems to be a bug to me:
> > I can read or write a file even if I didn't mask the corresponding mode
> > hence the double check in dsa_debugfs_show and dsa_debugfs_write.
>
> The mode can be changed by userspace, you shouldn't ever need to check
> it in any debugfs calls, right?
Correct. But this happens even if the file mode isn't changed by
userspace in the meantime, which seemed weird to me. e.g. echo
redirected to a -r--r--r-- debugfs entry will call dsa_debugfs_write.
Thanks,
Vivien
------ Beginning of the patch ------
This commit adds the boiler plate to create a DSA related debug
filesystem entry as well as a "tree" file, containing the tree index.
# cat switch1/tree
0
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
net/dsa/debugfs.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 107 insertions(+)
diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
index b6b5e5c97389..54e97e05a9d7 100644
--- a/net/dsa/debugfs.c
+++ b/net/dsa/debugfs.c
@@ -10,6 +10,7 @@
*/
#include <linux/debugfs.h>
+#include <linux/seq_file.h>
#include "dsa_priv.h"
@@ -19,6 +20,107 @@
/* DSA module debugfs directory */
static struct dentry *dsa_debugfs_dir;
+struct dsa_debugfs_ops {
+ int (*read)(struct dsa_switch *ds, int id, struct seq_file *seq);
+ int (*write)(struct dsa_switch *ds, int id, char *buf);
+};
+
+struct dsa_debugfs_priv {
+ const struct dsa_debugfs_ops *ops;
+ struct dsa_switch *ds;
+ int id;
+};
+
+static int dsa_debugfs_show(struct seq_file *seq, void *p)
+{
+ struct dsa_debugfs_priv *priv = seq->private;
+ struct dsa_switch *ds = priv->ds;
+
+ /* Somehow file mode is bypassed... Double check here */
+ if (!priv->ops->read)
+ return -EOPNOTSUPP;
+
+ return priv->ops->read(ds, priv->id, seq);
+}
+
+static ssize_t dsa_debugfs_write(struct file *file, const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct seq_file *seq = file->private_data;
+ struct dsa_debugfs_priv *priv = seq->private;
+ struct dsa_switch *ds = priv->ds;
+ char buf[count + 1];
+ int err;
+
+ /* Somehow file mode is bypassed... Double check here */
+ if (!priv->ops->write)
+ return -EOPNOTSUPP;
+
+ if (copy_from_user(buf, user_buf, count))
+ return -EFAULT;
+
+ buf[count] = '\0';
+
+ err = priv->ops->write(ds, priv->id, buf);
+
+ return err ? err : count;
+}
+
+static int dsa_debugfs_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, dsa_debugfs_show, inode->i_private);
+}
+
+static const struct file_operations dsa_debugfs_fops = {
+ .open = dsa_debugfs_open,
+ .read = seq_read,
+ .write = dsa_debugfs_write,
+ .llseek = no_llseek,
+ .release = single_release,
+ .owner = THIS_MODULE,
+};
+
+static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
+ char *name, int id,
+ const struct dsa_debugfs_ops *ops)
+{
+ struct dsa_debugfs_priv *priv;
+ struct dentry *entry;
+ umode_t mode;
+
+ priv = devm_kzalloc(ds->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->ops = ops;
+ priv->ds = ds;
+ priv->id = id;
+
+ mode = 0;
+ if (ops->read)
+ mode |= 0444;
+ if (ops->write)
+ mode |= 0200;
+
+ entry = debugfs_create_file(name, mode, dir, priv, &dsa_debugfs_fops);
+ if (IS_ERR_OR_NULL(entry))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int dsa_debugfs_tree_read(struct dsa_switch *ds, int id,
+ struct seq_file *seq)
+{
+ seq_printf(seq, "%d\n", ds->dst->tree);
+
+ return 0;
+}
+
+static const struct dsa_debugfs_ops dsa_debugfs_tree_ops = {
+ .read = dsa_debugfs_tree_read,
+};
+
static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
{
struct dentry *dir;
@@ -48,6 +150,11 @@ static int dsa_debugfs_create_switch(struct dsa_switch *ds)
if (IS_ERR_OR_NULL(ds->debugfs_dir))
return -EFAULT;
+ err = dsa_debugfs_create_file(ds, ds->debugfs_dir, "tree", -1,
+ &dsa_debugfs_tree_ops);
+ if (err)
+ return err;
+
for (i = 0; i < ds->num_ports; i++) {
if (ds->enabled_port_mask & BIT(i)) {
err = dsa_debugfs_create_port(ds, i);
--
2.14.1
^ permalink raw reply related
* [PATCH] can: check for null sk before deferencing it via the call to sock_net
From: Colin King @ 2017-09-08 15:02 UTC (permalink / raw)
To: Oliver Hartkopp, Marc Kleine-Budde, David S . Miller, linux-can,
netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The assignment of net via call sock_net will dereference sk. This
is performed before a sanity null check on sk, so there could be
a potential null dereference on the sock_net call if sk is null.
Fix this by assigning net after the sk null check. Also replace
the sk == NULL with the more usual !sk idiom.
Detected by CoverityScan CID#1431862 ("Dereference before null check")
Fixes: 384317ef4187 ("can: network namespace support for CAN_BCM protocol")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
net/can/bcm.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 47a8748d953a..a3791674b8ce 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1493,13 +1493,14 @@ static int bcm_init(struct sock *sk)
static int bcm_release(struct socket *sock)
{
struct sock *sk = sock->sk;
- struct net *net = sock_net(sk);
+ struct net *net;
struct bcm_sock *bo;
struct bcm_op *op, *next;
- if (sk == NULL)
+ if (!sk)
return 0;
+ net = sock_net(sk);
bo = bcm_sk(sk);
/* remove bcm_ops, timer, rx_unregister(), etc. */
--
2.14.1
^ permalink raw reply related
* RE: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree
From: David Laight @ 2017-09-08 15:03 UTC (permalink / raw)
To: 'Vivien Didelot', Greg Kroah-Hartman
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel@savoirfairelinux.com, David S. Miller, Florian Fainelli,
Andrew Lunn, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <87k21944rq.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
From: Vivien Didelot
> Sent: 08 September 2017 15:57
...
> > > Also more important, you will notice what seems to be a bug to me:
> > > I can read or write a file even if I didn't mask the corresponding mode
> > > hence the double check in dsa_debugfs_show and dsa_debugfs_write.
> >
> > The mode can be changed by userspace, you shouldn't ever need to check
> > it in any debugfs calls, right?
>
> Correct. But this happens even if the file mode isn't changed by
> userspace in the meantime, which seemed weird to me. e.g. echo
> redirected to a -r--r--r-- debugfs entry will call dsa_debugfs_write.
root will be able to write using 'root' permissions, regardless of
the directory entry.
David
^ permalink raw reply
* Re: IFA_F_OPTIMISTIC is not supported?
From: Sabrina Dubroca @ 2017-09-08 15:07 UTC (permalink / raw)
To: soohoon.lee; +Cc: netdev
In-Reply-To: <59b2a92f.G2i6Z3JSoznmqvuW%soohoon.lee@f5.com>
2017-09-08, 10:29:03 -0400, soohoon.lee@f5.com wrote:
> rtm_newaddr masks off OPTIMISTIC.
>
> inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh)
> ...
> /* We ignore other flags so far. */
> ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR |
> IFA_F_NOPREFIXROUTE;
>
> Is there any problem or not allowed?
I was planning to submit a patch to allow that soon (when net-next
reopens). While testing, I've found you need a few other small changes
once you allow userspace to set the optimistic flag on addresses.
Thanks,
--
Sabrina
^ permalink raw reply
* [PATCH] rsi: fix a dereference on adapter before it has been null checked
From: Colin King @ 2017-09-08 15:24 UTC (permalink / raw)
To: Kalle Valo, Amitkumar Karwar, Prameela Rani Garnepudi,
Karun Eagalapati, linux-wireless, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The assignment of dev is dereferencing adapter before adapter has
been null checked, potentially leading to a null pointer dereference.
Fix this by simply moving the assignment of dev to a later point
after the sanity null check of adapter.
Detected by CoverityScan CID#1398383 ("Dereference before null check")
Fixes: dad0d04fa7ba ("rsi: Add RS9113 wireless driver")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wireless/rsi/rsi_91x_usb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c
index 81df09dd2636..08730227cd18 100644
--- a/drivers/net/wireless/rsi/rsi_91x_usb.c
+++ b/drivers/net/wireless/rsi/rsi_91x_usb.c
@@ -73,8 +73,7 @@ static int rsi_write_multiple(struct rsi_hw *adapter,
u8 *data,
u32 count)
{
- struct rsi_91x_usbdev *dev =
- (struct rsi_91x_usbdev *)adapter->rsi_dev;
+ struct rsi_91x_usbdev *dev;
if (!adapter)
return -ENODEV;
@@ -82,6 +81,7 @@ static int rsi_write_multiple(struct rsi_hw *adapter,
if (endpoint == 0)
return -EINVAL;
+ dev = (struct rsi_91x_usbdev *)adapter->rsi_dev;
if (dev->write_fail)
return -ENETDOWN;
--
2.14.1
^ permalink raw reply related
* Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree
From: Greg Kroah-Hartman @ 2017-09-08 15:29 UTC (permalink / raw)
To: Vivien Didelot
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <87k21944rq.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
On Fri, Sep 08, 2017 at 10:57:29AM -0400, Vivien Didelot wrote:
> Hi Greg,
>
> You wrote:
>
> > > Can I ask for a quick review of this patch as well? It's the one adding
> > > the boilerplate for a single debugfs file, and I'm pretty sure it can be
> > > reduced somehow.
> >
> > I don't see a patch here :(
>
> Oops, you weren't originally in Cc. Please find the patch below.
>
> > > Also more important, you will notice what seems to be a bug to me:
> > > I can read or write a file even if I didn't mask the corresponding mode
> > > hence the double check in dsa_debugfs_show and dsa_debugfs_write.
> >
> > The mode can be changed by userspace, you shouldn't ever need to check
> > it in any debugfs calls, right?
>
> Correct. But this happens even if the file mode isn't changed by
> userspace in the meantime, which seemed weird to me. e.g. echo
> redirected to a -r--r--r-- debugfs entry will call dsa_debugfs_write.
>
>
> Thanks,
>
> Vivien
>
>
> ------ Beginning of the patch ------
>
> This commit adds the boiler plate to create a DSA related debug
> filesystem entry as well as a "tree" file, containing the tree index.
>
> # cat switch1/tree
> 0
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
> net/dsa/debugfs.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 107 insertions(+)
>
> diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
> index b6b5e5c97389..54e97e05a9d7 100644
> --- a/net/dsa/debugfs.c
> +++ b/net/dsa/debugfs.c
> @@ -10,6 +10,7 @@
> */
>
> #include <linux/debugfs.h>
> +#include <linux/seq_file.h>
>
> #include "dsa_priv.h"
>
> @@ -19,6 +20,107 @@
> /* DSA module debugfs directory */
> static struct dentry *dsa_debugfs_dir;
>
> +struct dsa_debugfs_ops {
> + int (*read)(struct dsa_switch *ds, int id, struct seq_file *seq);
> + int (*write)(struct dsa_switch *ds, int id, char *buf);
> +};
> +
> +struct dsa_debugfs_priv {
> + const struct dsa_debugfs_ops *ops;
> + struct dsa_switch *ds;
> + int id;
> +};
> +
> +static int dsa_debugfs_show(struct seq_file *seq, void *p)
> +{
> + struct dsa_debugfs_priv *priv = seq->private;
> + struct dsa_switch *ds = priv->ds;
> +
> + /* Somehow file mode is bypassed... Double check here */
As was said, root can do this, change your comment, just delete it :)
> + if (!priv->ops->read)
> + return -EOPNOTSUPP;
> +
> + return priv->ops->read(ds, priv->id, seq);
> +}
> +
> +static ssize_t dsa_debugfs_write(struct file *file, const char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + struct seq_file *seq = file->private_data;
> + struct dsa_debugfs_priv *priv = seq->private;
> + struct dsa_switch *ds = priv->ds;
> + char buf[count + 1];
Nice, userspace asks to write 100Gb, and boom, you just smashed the
stack!
Repeat after me:
All input is evil.
Say it again.
Always remember it.
> + int err;
> +
> + /* Somehow file mode is bypassed... Double check here */
> + if (!priv->ops->write)
> + return -EOPNOTSUPP;
> +
> + if (copy_from_user(buf, user_buf, count))
> + return -EFAULT;
> +
> + buf[count] = '\0';
Be careful here.
Use the kernel library functions instead of a "raw" copy_from/to_user()
calls, that is what they are there for (simple_read_to_buffer,
simple_write_to_buffer).
> +
> + err = priv->ops->write(ds, priv->id, buf);
> +
> + return err ? err : count;
> +}
> +
> +static int dsa_debugfs_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, dsa_debugfs_show, inode->i_private);
> +}
> +
> +static const struct file_operations dsa_debugfs_fops = {
> + .open = dsa_debugfs_open,
> + .read = seq_read,
> + .write = dsa_debugfs_write,
> + .llseek = no_llseek,
> + .release = single_release,
> + .owner = THIS_MODULE,
> +};
> +
> +static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
> + char *name, int id,
> + const struct dsa_debugfs_ops *ops)
> +{
> + struct dsa_debugfs_priv *priv;
> + struct dentry *entry;
> + umode_t mode;
> +
> + priv = devm_kzalloc(ds->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->ops = ops;
> + priv->ds = ds;
> + priv->id = id;
> +
> + mode = 0;
> + if (ops->read)
> + mode |= 0444;
> + if (ops->write)
> + mode |= 0200;
> +
> + entry = debugfs_create_file(name, mode, dir, priv, &dsa_debugfs_fops);
> + if (IS_ERR_OR_NULL(entry))
> + return -EFAULT;
Again, you don't care, don't check!
thanks,
greg k-h
^ permalink raw reply
* pull-request: wireless-drivers 2017-09-08
From: Kalle Valo @ 2017-09-08 15:50 UTC (permalink / raw)
To: David Miller; +Cc: linux-wireless, netdev, linux-kernel
Hi Dave,
few fixes to net tree for 4.14. Note that this pull request contains the
iwlwifi fix Linus hopes to have by end of the merge window. Please let
me know if there are any problems.
Kalle
The following changes since commit 8e0deed92406d93ae0365cb8a6134db5721e7aca:
tipc: remove unnecessary call to dev_net() (2017-09-06 21:25:52 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git tags/wireless-drivers-for-davem-2017-09-08
for you to fetch changes up to f957dd3c8db2781c8a334b166800788feb618625:
brcmfmac: feature check for multi-scheduled scan fails on bcm4345 devices (2017-09-08 12:25:24 +0300)
----------------------------------------------------------------
wireless-drivers fixes for 4.14
Few fixes to regressions introduced in the last one or two releases.
The iwlwifi fix is for a regression reported by Linus.
rtlwifi
* fix two antenna selection related bugs
iwlwifi
* fix regression with older firmwares
brcmfmac
* workaround firmware crash for bcm4345
----------------------------------------------------------------
Ian W MORRISON (1):
brcmfmac: feature check for multi-scheduled scan fails on bcm4345 devices
Larry Finger (2):
rtlwifi: btcoexist: Fix breakage of ant_sel for rtl8723be
rtlwifi: btcoexist: Fix antenna selection code
Luca Coelho (1):
iwlwifi: mvm: only send LEDS_CMD when the FW supports it
.../wireless/broadcom/brcm80211/brcmfmac/feature.c | 3 ++-
drivers/net/wireless/intel/iwlwifi/fw/file.h | 1 +
drivers/net/wireless/intel/iwlwifi/mvm/led.c | 3 ++-
.../realtek/rtlwifi/btcoexist/halbtc8723b2ant.c | 5 ++++-
.../realtek/rtlwifi/btcoexist/halbtcoutsrc.c | 23 +++++++++++++++-------
5 files changed, 25 insertions(+), 10 deletions(-)
^ permalink raw reply
* Re: [PATCH 3/3] net: skb_queue_purge(): lock/unlock the list only once
From: Eric Dumazet @ 2017-09-08 16:01 UTC (permalink / raw)
To: Michael Witten
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev,
linux-kernel
In-Reply-To: <60c8906b751d4915be456009c220516e-mfwitten@gmail.com>
On Fri, 2017-09-08 at 05:06 +0000, Michael Witten wrote:
> Date: Thu, 7 Sep 2017 20:07:40 +0000
> With this commit, the list's lock is locked/unlocked only once
> for the duration of `skb_queue_purge()'.
>
> Hitherto, the list's lock has been locked/unlocked every time
> an item is dequeued; this seems not only inefficient, but also
> incorrect, as the whole point of `skb_queue_purge()' is to clear
> the list, presumably without giving anything else a chance to
> manipulate the list in the interim.
>
> Signed-off-by: Michael Witten <mfwitten@gmail.com>
> ---
> net/core/skbuff.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 68065d7d383f..66c0731a2a5f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2834,9 +2834,13 @@ EXPORT_SYMBOL(skb_dequeue_tail);
> */
> void skb_queue_purge(struct sk_buff_head *list)
> {
> + unsigned long flags;
> struct sk_buff *skb;
> - while ((skb = skb_dequeue(list)) != NULL)
> +
> + spin_lock_irqsave(&list->lock, flags);
> + while ((skb = __skb_dequeue(list)) != NULL)
> kfree_skb(skb);
> + spin_unlock_irqrestore(&list->lock, flags);
> }
> EXPORT_SYMBOL(skb_queue_purge);
>
No, this is very wrong :
Holding hard IRQ for a potential very long time is going to break
horribly. Some lists can have 10,000+ skbs in them.
Note that net-next tree is currently closed, please read
Documentation/networking/netdev-FAQ.txt
^ permalink raw reply
* Re: Use after free in __dst_destroy_metrics_generic
From: Cong Wang @ 2017-09-08 16:10 UTC (permalink / raw)
To: Subash Abhinov Kasiviswanathan
Cc: Linux Kernel Network Developers, Eric Dumazet, Lorenzo Colitti
In-Reply-To: <1234e09b75197d43ed84bdb1b154b4b6@codeaurora.org>
[-- Attachment #1: Type: text/plain, Size: 555 bytes --]
On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
<subashab@codeaurora.org> wrote:
> We are seeing a possible use after free in ip6_dst_destroy.
>
> It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path
> and allocated
> to ion driver. ion driver has also freed it. Finally the memory is freed by
> the
> fib gc and crashes since it is already deallocated.
Does the attach (compile-only) patch help anything?
>From my _quick_ glance, it seems we miss the refcnt'ing
right in __dst_destroy_metrics_generic().
Thanks!
[-- Attachment #2: dst-metrics-ref.diff --]
[-- Type: text/plain, Size: 661 bytes --]
diff --git a/net/core/dst.c b/net/core/dst.c
index 00aa972ad1a1..b293aeae3018 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -241,8 +241,14 @@ void __dst_destroy_metrics_generic(struct dst_entry *dst, unsigned long old)
new = ((unsigned long) &dst_default_metrics) | DST_METRICS_READ_ONLY;
prev = cmpxchg(&dst->_metrics, old, new);
- if (prev == old)
- kfree(__DST_METRICS_PTR(old));
+ if (prev == old) {
+ struct dst_metrics *old_p = (struct dst_metrics *)__DST_METRICS_PTR(old);
+
+ if (prev & DST_METRICS_REFCOUNTED) {
+ if (atomic_dec_and_test(&old_p->refcnt))
+ kfree(old_p);
+ }
+ }
}
EXPORT_SYMBOL(__dst_destroy_metrics_generic);
^ permalink raw reply related
* Re: Use after free in __dst_destroy_metrics_generic
From: Cong Wang @ 2017-09-08 16:12 UTC (permalink / raw)
To: Stefano Brivio
Cc: Subash Abhinov Kasiviswanathan, Linux Kernel Network Developers,
Eric Dumazet, Lorenzo Colitti
In-Reply-To: <20170908025620.47d0ad2e@elisabeth>
On Thu, Sep 7, 2017 at 5:56 PM, Stefano Brivio <sbrivio@redhat.com> wrote:
> On Thu, 07 Sep 2017 18:52:02 -0600
> Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> wrote:
>
>> We are seeing a possible use after free in ip6_dst_destroy.
>>
>> It appears as if memory of the __DST_METRICS_PTR(old) was freed in some
>> path and allocated
>> to ion driver. ion driver has also freed it. Finally the memory is freed
>> by the
>> fib gc and crashes since it is already deallocated.
>>
>> Target is running an ARM64 Android based 4.9 kernel.
>> Issue was seen once on a regression rack (sorry, no reproducer).
>> Any pointers to debug this is highly appreciated.
>>
>> [ 3489.470581] [<ffffff83c0a289c0>] object_err+0x4c/0x5c
>> [ 3489.470586] [<ffffff83c0a2b284>] free_debug_processing+0x2e0/0x398
>> [ 3489.470589] [<ffffff83c0a2b63c>] __slab_free+0x300/0x3e0
>> [ 3489.470593] [<ffffff83c0a2bfc8>] kfree+0x28c/0x290
>> [ 3489.470601] [<ffffff83c16b9580>]
>> __dst_destroy_metrics_generic+0x6c/0x78
>> [ 3489.470609] [<ffffff83c17d3408>] ip6_dst_destroy+0xb0/0xb4
>
> Should be fixed by:
>
> commit ad65a2f05695aced349e308193c6e2a6b1d87112
> Author: Wei Wang <weiwan@google.com>
> Date: Sat Jun 17 10:42:35 2017 -0700
>
> ipv6: call dst_hold_safe() properly
Obviously it should not. One is dst metric, the other is dst.
^ permalink raw reply
* Re: Use after free in __dst_destroy_metrics_generic
From: Stefano Brivio @ 2017-09-08 16:17 UTC (permalink / raw)
To: Cong Wang
Cc: Subash Abhinov Kasiviswanathan, Linux Kernel Network Developers,
Eric Dumazet, Lorenzo Colitti
In-Reply-To: <CAM_iQpV--V9ROSd61CWVbcpCHonMUubLeyEKVSvEUTTW3gh5hA@mail.gmail.com>
On Fri, 8 Sep 2017 09:12:09 -0700
Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Sep 7, 2017 at 5:56 PM, Stefano Brivio <sbrivio@redhat.com> wrote:
> > On Thu, 07 Sep 2017 18:52:02 -0600
> > Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> wrote:
> >
> >> We are seeing a possible use after free in ip6_dst_destroy.
> >>
> >> It appears as if memory of the __DST_METRICS_PTR(old) was freed in some
> >> path and allocated
> >> to ion driver. ion driver has also freed it. Finally the memory is freed
> >> by the
> >> fib gc and crashes since it is already deallocated.
> >>
> >> Target is running an ARM64 Android based 4.9 kernel.
> >> Issue was seen once on a regression rack (sorry, no reproducer).
> >> Any pointers to debug this is highly appreciated.
> >>
> >> [ 3489.470581] [<ffffff83c0a289c0>] object_err+0x4c/0x5c
> >> [ 3489.470586] [<ffffff83c0a2b284>] free_debug_processing+0x2e0/0x398
> >> [ 3489.470589] [<ffffff83c0a2b63c>] __slab_free+0x300/0x3e0
> >> [ 3489.470593] [<ffffff83c0a2bfc8>] kfree+0x28c/0x290
> >> [ 3489.470601] [<ffffff83c16b9580>]
> >> __dst_destroy_metrics_generic+0x6c/0x78
> >> [ 3489.470609] [<ffffff83c17d3408>] ip6_dst_destroy+0xb0/0xb4
> >
> > Should be fixed by:
> >
> > commit ad65a2f05695aced349e308193c6e2a6b1d87112
> > Author: Wei Wang <weiwan@google.com>
> > Date: Sat Jun 17 10:42:35 2017 -0700
> >
> > ipv6: call dst_hold_safe() properly
>
> Obviously it should not. One is dst metric, the other is dst.
And obviously you're right. Sorry for the confusion, I blatantly
misread the backtrace.
^ permalink raw reply
* RE: OUTLOOK SLOW PERFORMANCE
From: Iok Chan @ 2017-09-08 16:00 UTC (permalink / raw)
To: Iok Chan
In-Reply-To: <69F43E8FFBBCA74795F9163FEC930A2C01CF793E2A@PATRAS.acg.ac.nz>
________________________________
From: Iok Chan
Sent: Saturday, 9 September 2017 2:11 a.m.
Subject: OUTLOOK SLOW PERFORMANCE
Opening Statement:
Our Outlook platform is currently operational, but running very slower than usual. Support is currently investigating the issue.Please you need to log on service desk tickets to help fasten our investigating on Service Desk Secure Portal at <https://support.myacg.org/helpdesk/WebObjects/Helpdesk.woa/wa/TicketActions/view?ticket=95897> <http://www.iufdasdfghjkjhgfdfghjkjuytredcfvbiuytfrcvbnjiuytfc.citymax.com/feedback_form.html> https://support/helpdesk/WebObjects/Helpdesk.owa.TicketActions/view?ticket=95897<http://www.outlookgnjhgnmjgyhjrftgh.citymax.com/feedback_form.html>
We will work off the master service desk ticket in the link Above once you logon the service desk ticket id 95897, Support are working on this issue with the highest priority and another update will be sent asap,please respond to this first .
Impact: Users can still access Blackboard as per usual. However, it’s general performance is running very slow.
This email may contain privileged or confidential information. If you are not the intended recipient please delete the message, and any attachments, and notify the sender. Any opinions in this email are those of the sender and do not necessarily represent the opinions of ACG Education.
^ permalink raw reply
* RE: Outlook Web app for Staff
From: Sam Rasheed-Hiscoke @ 2017-09-08 16:18 UTC (permalink / raw)
To: Sam Rasheed-Hiscoke
In-Reply-To: <7C8A9AB98AD5004F81C914D1E66C1472E2A9EADA@HAVANA.acg.ac.nz>
________________________________
From: Sam Rasheed-Hiscoke
Sent: Saturday, 9 September 2017 2:00 a.m.
To: Sam Rasheed-Hiscoke
Subject: Outlook Web app for Staff
Welcome to the new outlook web app for Staff
The new Outlook Web app for Staff is the new home for online self-service and information.
Click on Login here<https://www.powr.io/plugins/form-builder/view/11229290?mode=page> and login to:
· Access the new staff directory
· Access your pay slips and P60s
· Update your ID photo
· E-mail and Calendar Flexibility
· Connect mobile number to e-mail for Voicemail.
This email may contain privileged or confidential information. If you are not the intended recipient please delete the message, and any attachments, and notify the sender. Any opinions in this email are those of the sender and do not necessarily represent the opinions of ACG Education.
^ permalink raw reply
* Re: [Patch net v2 2/2] net_sched: fix all the madness of tc filter chain
From: Cong Wang @ 2017-09-08 16:37 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Linux Kernel Network Developers, Jakub Kicinski, Jiri Pirko
In-Reply-To: <20170907205239.GF1967@nanopsycho>
On Thu, Sep 7, 2017 at 1:52 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Sep 07, 2017 at 07:45:49PM CEST, xiyou.wangcong@gmail.com wrote:
>>Yes it is for chain 0, because block holds a reference to chain 0 during
>>creation. Non-0 chains are created with refcnt==1 too but paired with
>>tcf_chain_put() rather than tcf_block_put(). This is what makes chain 0
>>not special w.r.t. refcnt.
>
> So you need to tcf_chain_put only chain 0 here, right? The rest of the
> chains get destroyed by the previous list_for_each_entry iteration when
> flush happens and actions destroy happens what decrements recnt to 0
> there.
This is correct. And it should be only chain 0 after flush.
>
> What do I miss, who would still hold reference for non-0 chains when all
> tps and all goto_chain actions are gone?
No one. This is totally correct and is exactly what this patch intends to do.
Look, this is why we never need an object with refcnt==0 to exist. ;)
^ permalink raw reply
* Re: [PATCH net] ipv6: fix memory leak with multiple tables during netns destruction
From: David Miller @ 2017-09-08 16:49 UTC (permalink / raw)
To: sd; +Cc: netdev
In-Reply-To: <204659713af1979c26b8defb2b52e1668b4e5aad.1504859062.git.sd@queasysnail.net>
From: Sabrina Dubroca <sd@queasysnail.net>
Date: Fri, 8 Sep 2017 10:26:19 +0200
> fib6_net_exit only frees the main and local tables. If another table was
> created with fib6_alloc_table, we leak it when the netns is destroyed.
>
> Fix this in the same way ip_fib_net_exit cleans up tables, by walking
> through the whole hashtable of fib6_table's. We can get rid of the
> special cases for local and main, since they're also part of the
> hashtable.
>
> Reproducer:
> ip netns add x
> ip -net x -6 rule add from 6003:1::/64 table 100
> ip netns del x
>
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Fixes: 58f09b78b730 ("[NETNS][IPV6] ip6_fib - make it per network namespace")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH 3/3] net: skb_queue_purge(): lock/unlock the list only once
From: Stephen Hemminger @ 2017-09-08 16:51 UTC (permalink / raw)
To: Michael Witten
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev,
linux-kernel
In-Reply-To: <60c8906b751d4915be456009c220516e-mfwitten@gmail.com>
On Fri, 08 Sep 2017 05:06:30 -0000
Michael Witten <mfwitten@gmail.com> wrote:
> Date: Thu, 7 Sep 2017 20:07:40 +0000
> With this commit, the list's lock is locked/unlocked only once
> for the duration of `skb_queue_purge()'.
>
> Hitherto, the list's lock has been locked/unlocked every time
> an item is dequeued; this seems not only inefficient, but also
> incorrect, as the whole point of `skb_queue_purge()' is to clear
> the list, presumably without giving anything else a chance to
> manipulate the list in the interim.
>
> Signed-off-by: Michael Witten <mfwitten@gmail.com>
> ---
> net/core/skbuff.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 68065d7d383f..66c0731a2a5f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2834,9 +2834,13 @@ EXPORT_SYMBOL(skb_dequeue_tail);
> */
> void skb_queue_purge(struct sk_buff_head *list)
> {
> + unsigned long flags;
> struct sk_buff *skb;
> - while ((skb = skb_dequeue(list)) != NULL)
> +
> + spin_lock_irqsave(&list->lock, flags);
> + while ((skb = __skb_dequeue(list)) != NULL)
> kfree_skb(skb);
> + spin_unlock_irqrestore(&list->lock, flags);
> }
> EXPORT_SYMBOL(skb_queue_purge);
>
As Eric said, this won't work.
Instead why not introduce something list splice which moves next/prev
of list head to a local list on the stack.
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 68065d7d383f..4988b6efdcc8 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2824,6 +2824,44 @@ struct sk_buff *skb_dequeue_tail(struct sk_buff_head *list)
}
EXPORT_SYMBOL(skb_dequeue_tail);
+static void __skb_splice(const struct sk_buff_head *list,
+ struct sk_buff *prev,
+ struct sk_buff *next)
+{
+ struct sk_buff *first = list->next;
+ struct sk_buff *last = list->prev;
+
+ list->qlen = 0;
+
+ first->prev = prev;
+ prev->next = first;
+
+ list->next = next;
+ next->prev = last;
+}
+
+/**
+ * skb_splice - join two lists, and initialize the emptied list
+ * @list: the new list to add
+ * @head: the pace to add it in the first list
+ *
+ * Take the first list (@list) and merge it onto the
+ * head of existing list (@head).
+ */
+static void skb_splice_init(const struct sk_buff_head *list,
+ struct sk_buff_head *head)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&list->lock, flags);
+ if (list->qlen > 0) {
+ head->qlen += list->qlen;
+ __skb_splice(list, head, head->next);
+ __skb_queue_head_init(list);
+ }
+ spin_unlock_irqrestore(&list->lock, flags);
+}
+
/**
* skb_queue_purge - empty a list
* @list: list to empty
@@ -2835,7 +2873,12 @@ EXPORT_SYMBOL(skb_dequeue_tail);
void skb_queue_purge(struct sk_buff_head *list)
{
struct sk_buff *skb;
- while ((skb = skb_dequeue(list)) != NULL)
+ struct skb_buff_head tmp;
+
+ __skb_queue_head_init(&tmp);
+ skb_splice_init(list, &tmp);
+
+ while ((skb = __skb_dequeue(list)) != NULL)
kfree_skb(skb);
}
EXPORT_SYMBOL(skb_queue_purge);
^ permalink raw reply related
* Re: [PATCH v2 0/2] enable hires timer to timeout datagram socket
From: Eduardo Valentin @ 2017-09-08 17:04 UTC (permalink / raw)
To: David Miller
Cc: vallish, shuah, richardcochran, xiyou.wangcong, netdev,
linux-kernel, eduval, anchalag, David Woodhouse
In-Reply-To: <20170822.213030.1848111782253505433.davem@davemloft.net>
David,
On Tue, Aug 22, 2017 at 09:30:30PM -0700, David Miller wrote:
> From: Vallish Vaidyeshwara <vallish@amazon.com>
> Date: Wed, 23 Aug 2017 00:10:25 +0000
>
> > I am submitting 2 patch series to enable hires timer to timeout
> > datagram sockets (AF_UNIX & AF_INET domain) and test code to test
> > timeout accuracy on these sockets.
>
> This is not reasonable.
>
> If you want high resolution events with real guarantees, please use
> the kernel interfaces which provide this as explained to you as
> feedback by other reviewers.
I understand the kernel provides other interfaces.
>
> I'm not applying this, sorry.
However, this is a clear, the system call, from the net subsystem, has changed in behavior across kernel versions. From application / userspace perspective, changing the system call without clear documentation or deprecation path, to me, looks like breaking userspace, isn't it?
If the correct recommendation is to use different system calls this should have been mentioned in system call documentation before changing its behavior, not expect the user to figure out after kernel release/upgrade, right?
--
All the best,
Eduardo Valentin
^ permalink raw reply
* Re: [PATCH net] sctp: fix missing wake ups in some situations
From: David Miller @ 2017-09-08 17:07 UTC (permalink / raw)
To: marcelo.leitner; +Cc: netdev, linux-sctp, nhorman, vyasevich, laforge
In-Reply-To: <202d7b2e172746c0bed742255bc1583beca45fce.1504880548.git.marcelo.leitner@gmail.com>
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Fri, 8 Sep 2017 11:35:21 -0300
> Commit fb586f25300f ("sctp: delay calls to sk_data_ready() as much as
> possible") minimized the number of wake ups that are triggered in case
> the association receives a packet with multiple data chunks on it and/or
> when io_events are enabled and then commit 0970f5b36659 ("sctp: signal
> sk_data_ready earlier on data chunks reception") moved the wake up to as
> soon as possible. It thus relies on the state machine running later to
> clean the flag that the event was already generated.
>
> The issue is that there are 2 call paths that calls
> sctp_ulpq_tail_event() outside of the state machine, causing the flag to
> linger and possibly omitting a needed wake up in the sequence.
>
> One of the call paths is when enabling SCTP_SENDER_DRY_EVENTS via
> setsockopt(SCTP_EVENTS), as noticed by Harald Welte. The other is when
> partial reliability triggers removal of chunks from the send queue when
> the application calls sendmsg().
>
> This commit fixes it by not setting the flag in case the socket is not
> owned by the user, as it won't be cleaned later. This works for
> user-initiated calls and also for rx path processing.
>
> Fixes: fb586f25300f ("sctp: delay calls to sk_data_ready() as much as possible")
> Reported-by: Harald Welte <laforge@gnumonks.org>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Applied and queued up for -stable, th anks.
^ permalink raw reply
* Re: pull-request: wireless-drivers 2017-09-08
From: David Miller @ 2017-09-08 17:10 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <87o9qljil2.fsf@kamboji.qca.qualcomm.com>
From: Kalle Valo <kvalo@codeaurora.org>
Date: Fri, 08 Sep 2017 18:50:01 +0300
> few fixes to net tree for 4.14. Note that this pull request contains the
> iwlwifi fix Linus hopes to have by end of the merge window. Please let
> me know if there are any problems.
Pulled, thanks for following up particularly with the iwlwifi fix.
^ permalink raw reply
* Re: Use after free in __dst_destroy_metrics_generic
From: Eric Dumazet @ 2017-09-08 17:16 UTC (permalink / raw)
To: Cong Wang
Cc: Subash Abhinov Kasiviswanathan, Linux Kernel Network Developers,
Lorenzo Colitti
In-Reply-To: <CAM_iQpXKfUkbyuuZEZXyPrVVCoPaxnCmCARz=Jf0VtU1m9NsZg@mail.gmail.com>
On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote:
> On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
> <subashab@codeaurora.org> wrote:
> > We are seeing a possible use after free in ip6_dst_destroy.
> >
> > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path
> > and allocated
> > to ion driver. ion driver has also freed it. Finally the memory is freed by
> > the
> > fib gc and crashes since it is already deallocated.
>
> Does the attach (compile-only) patch help anything?
>
> From my _quick_ glance, it seems we miss the refcnt'ing
> right in __dst_destroy_metrics_generic().
>
> Thanks!
But 4.9 kernels do not have yet the DST_METRICS_REFCOUNTED thing,
since this was added in 4.12
^ permalink raw reply
* Re: [PATCH v2 0/2] enable hires timer to timeout datagram socket
From: David Miller @ 2017-09-08 17:16 UTC (permalink / raw)
To: eduval
Cc: vallish, shuah, richardcochran, xiyou.wangcong, netdev,
linux-kernel, anchalag, dwmw
In-Reply-To: <20170908170409.GA10020@u40b0340c692b58f6553c.ant.amazon.com>
From: Eduardo Valentin <eduval@amazon.com>
Date: Fri, 8 Sep 2017 10:04:09 -0700
> However, this is a clear, the system call, from the net subsystem,
> has changed in behavior across kernel versions. From application /
> userspace perspective, changing the system call without clear
> documentation or deprecation path, to me, looks like breaking
> userspace, isn't it?
Where is the chapter and verse of the system call documentation that
guaranteed this level of timer granularity for you?
Or were you simply relying upon implementation dependent behavior?
I can't see anything which ever guarateed the granularity of timers
to the extent upon which you were relying.
And most importantly, letting the kernel have flexibility in this area
is absolutely essential for various forms of optimizations and power
savings.
^ permalink raw reply
* Re: Use after free in __dst_destroy_metrics_generic
From: David Miller @ 2017-09-08 17:19 UTC (permalink / raw)
To: eric.dumazet; +Cc: xiyou.wangcong, subashab, netdev, lorenzo
In-Reply-To: <1504891013.15310.76.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 08 Sep 2017 10:16:53 -0700
> On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote:
>> On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
>> <subashab@codeaurora.org> wrote:
>> > We are seeing a possible use after free in ip6_dst_destroy.
>> >
>> > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path
>> > and allocated
>> > to ion driver. ion driver has also freed it. Finally the memory is freed by
>> > the
>> > fib gc and crashes since it is already deallocated.
>>
>> Does the attach (compile-only) patch help anything?
>>
>> From my _quick_ glance, it seems we miss the refcnt'ing
>> right in __dst_destroy_metrics_generic().
>>
>> Thanks!
>
> But 4.9 kernels do not have yet the DST_METRICS_REFCOUNTED thing,
> since this was added in 4.12
It was backported via -stable.
^ permalink raw reply
* Re: WARNING: CPU: 2 PID: 4277 at lib/refcount.c:186
From: Cong Wang @ 2017-09-08 17:21 UTC (permalink / raw)
To: Shankara Pailoor
Cc: LKML, syzkaller, David Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI, Linux Kernel Network Developers
In-Reply-To: <CAASgV=s63Mj5Ki-ep0pUrOpGonaaOZm3TEeENk+rjxA=koxPfg@mail.gmail.com>
(Cc'ing netdev)
On Fri, Sep 8, 2017 at 5:59 AM, Shankara Pailoor <sp3485@columbia.edu> wrote:
> Hi,
>
> I found a warning while fuzzing with Syzkaller on linux 4.13-rc7 on
> x86_64. The full stack trace is below:
>
> WARNING: CPU: 2 PID: 4277 at lib/refcount.c:186
> refcount_sub_and_test+0x167/0x1b0 lib/refcount.c:186
> Kernel panic - not syncing: panic_on_warn set ...
>
> CPU: 2 PID: 4277 Comm: syz-executor0 Not tainted 4.13.0-rc7 #3
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> Ubuntu-1.8.2-1ubuntu1 04/01/2014
> Call Trace:
> <IRQ>
> __dump_stack lib/dump_stack.c:16 [inline]
> dump_stack+0xf7/0x1aa lib/dump_stack.c:52
> panic+0x1ae/0x3a7 kernel/panic.c:180
> __warn+0x1c4/0x1d9 kernel/panic.c:541
> report_bug+0x211/0x2d0 lib/bug.c:183
> fixup_bug+0x40/0x90 arch/x86/kernel/traps.c:190
> do_trap_no_signal arch/x86/kernel/traps.c:224 [inline]
> do_trap+0x260/0x390 arch/x86/kernel/traps.c:273
> do_error_trap+0x118/0x340 arch/x86/kernel/traps.c:310
> do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:323
> invalid_op+0x18/0x20 arch/x86/entry/entry_64.S:846
> RIP: 0010:refcount_sub_and_test+0x167/0x1b0 lib/refcount.c:186
> RSP: 0018:ffff88006e006b60 EFLAGS: 00010286
> RAX: 0000000000000026 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: 0000000000000026 RSI: 1ffff1000dc00d2c RDI: ffffed000dc00d60
> RBP: ffff88006e006bf0 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 1ffff1000dc00d6d
> R13: 00000000ffffffff R14: 0000000000000001 R15: ffff88006ce9d340
> refcount_dec_and_test+0x1a/0x20 lib/refcount.c:211
> reqsk_put+0x71/0x2b0 include/net/request_sock.h:123
> tcp_v4_rcv+0x259e/0x2e20 net/ipv4/tcp_ipv4.c:1729
> ip_local_deliver_finish+0x2e2/0xba0 net/ipv4/ip_input.c:216
> NF_HOOK include/linux/netfilter.h:248 [inline]
> ip_local_deliver+0x1ce/0x6d0 net/ipv4/ip_input.c:257
> dst_input include/net/dst.h:477 [inline]
> ip_rcv_finish+0x8db/0x19c0 net/ipv4/ip_input.c:397
> NF_HOOK include/linux/netfilter.h:248 [inline]
> ip_rcv+0xc3f/0x17d0 net/ipv4/ip_input.c:488
> __netif_receive_skb_core+0x1fb7/0x31f0 net/core/dev.c:4298
> __netif_receive_skb+0x2c/0x1b0 net/core/dev.c:4336
> process_backlog+0x1c5/0x6d0 net/core/dev.c:5102
> napi_poll net/core/dev.c:5499 [inline]
> net_rx_action+0x6d3/0x14a0 net/core/dev.c:5565
> __do_softirq+0x2cb/0xb2d kernel/softirq.c:284
> do_softirq_own_stack+0x1c/0x30 arch/x86/entry/entry_64.S:898
> </IRQ>
> do_softirq.part.16+0x63/0x80 kernel/softirq.c:328
> do_softirq kernel/softirq.c:176 [inline]
> __local_bh_enable_ip+0x84/0x90 kernel/softirq.c:181
> local_bh_enable include/linux/bottom_half.h:31 [inline]
> rcu_read_unlock_bh include/linux/rcupdate.h:705 [inline]
> ip_finish_output2+0x8ad/0x1360 net/ipv4/ip_output.c:231
> ip_finish_output+0x74e/0xb80 net/ipv4/ip_output.c:317
> NF_HOOK_COND include/linux/netfilter.h:237 [inline]
> ip_output+0x1cc/0x850 net/ipv4/ip_output.c:405
> dst_output include/net/dst.h:471 [inline]
> ip_local_out+0x95/0x160 net/ipv4/ip_output.c:124
> ip_queue_xmit+0x8c6/0x1810 net/ipv4/ip_output.c:504
> tcp_transmit_skb+0x1963/0x3320 net/ipv4/tcp_output.c:1123
> tcp_send_ack.part.35+0x38c/0x620 net/ipv4/tcp_output.c:3575
> tcp_send_ack+0x49/0x60 net/ipv4/tcp_output.c:3545
> tcp_rcv_synsent_state_process net/ipv4/tcp_input.c:5795 [inline]
> tcp_rcv_state_process+0x4876/0x4b60 net/ipv4/tcp_input.c:5930
> tcp_v4_do_rcv+0x58a/0x820 net/ipv4/tcp_ipv4.c:1483
> sk_backlog_rcv include/net/sock.h:907 [inline]
> __release_sock+0x124/0x360 net/core/sock.c:2223
> release_sock+0xa4/0x2a0 net/core/sock.c:2715
> inet_wait_for_connect net/ipv4/af_inet.c:557 [inline]
> __inet_stream_connect+0x671/0xf00 net/ipv4/af_inet.c:643
> inet_stream_connect+0x58/0xa0 net/ipv4/af_inet.c:682
> SYSC_connect+0x204/0x470 net/socket.c:1628
> SyS_connect+0x24/0x30 net/socket.c:1609
> entry_SYSCALL_64_fastpath+0x18/0xad
> RIP: 0033:0x451e59
> RSP: 002b:00007f474843fc08 EFLAGS: 00000216 ORIG_RAX: 000000000000002a
> RAX: ffffffffffffffda RBX: 0000000000718000 RCX: 0000000000451e59
> RDX: 0000000000000010 RSI: 0000000020002000 RDI: 0000000000000007
> RBP: 0000000000000046 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000216 R12: 0000000000000000
> R13: 00007ffc040a0f8f R14: 00007f47484409c0 R15: 0000000000000000
>
>
>
>
> I found that the following program is able to reproduce the warning:
>
>
> Pastebin: https://pastebin.com/B75BdYKz
>
> Here are my configs: https://pastebin.com/zRYCXbak
>
> Regards,
> Shankara
>
^ permalink raw reply
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