* Re: [PATCH net-next 1/1] ipvlan: Initial check-in of the IPVLAN driver.
From: David Miller @ 2014-11-11 23:19 UTC (permalink / raw)
To: cwang
Cc: maheshb, netdev, edumazet, maze, chavey, thockin, brandon.philips,
xemul
In-Reply-To: <CAHA+R7OZxCLFYetGHONWkL7uzGur=xSc=ASBT4i5EEeS2Xzwog@mail.gmail.com>
From: Cong Wang <cwang@twopensource.com>
Date: Tue, 11 Nov 2014 15:12:27 -0800
> On Tue, Nov 11, 2014 at 2:29 PM, Mahesh Bandewar <maheshb@google.com> wrote:
>> This driver is very similar to the macvlan driver except that it
>> uses L3 on the frame to determine the logical interface while
>> functioning as packet dispatcher. It inherits L2 of the master
>> device hence the packets on wire will have the same L2 for all
>> the packets originating from all virtual devices off of the same
>> master device.
>
> Why do we need this from the beginning?
> IOW, what problem does this solve while macvlan doesn't?
macvlan has several built-in limitations, which IP VLAN absolutely
does not have.
Eric Dumazet spoke about this at the networking track at the kernel
summit in Chicago, maybe he or another person working on this can
chime in.
^ permalink raw reply
* Re: [PATCH net-next 1/1] ipvlan: Initial check-in of the IPVLAN driver.
From: Hannes Frederic Sowa @ 2014-11-11 23:22 UTC (permalink / raw)
To: Cong Wang
Cc: Mahesh Bandewar, netdev, Eric Dumazet, Maciej Zenczykowski,
Laurent Chavey, Tim Hockin, David Miller, Brandon Philips,
Pavel Emelianov
In-Reply-To: <CAHA+R7OZxCLFYetGHONWkL7uzGur=xSc=ASBT4i5EEeS2Xzwog@mail.gmail.com>
On Di, 2014-11-11 at 15:12 -0800, Cong Wang wrote:
> On Tue, Nov 11, 2014 at 2:29 PM, Mahesh Bandewar <maheshb@google.com> wrote:
> > This driver is very similar to the macvlan driver except that it
> > uses L3 on the frame to determine the logical interface while
> > functioning as packet dispatcher. It inherits L2 of the master
> > device hence the packets on wire will have the same L2 for all
> > the packets originating from all virtual devices off of the same
> > master device.
>
> Why do we need this from the beginning?
> IOW, what problem does this solve while macvlan doesn't?
I think it is good to reduce the number of mac addresses before a NIC
switches into promisc mode.
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH net 0/2] net: bcmgenet: power management related fixes
From: David Miller @ 2014-11-11 23:24 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev
In-Reply-To: <1415671581-2835-1-git-send-email-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 10 Nov 2014 18:06:19 -0800
> These two patches fixes issues seen while testing power management on
> platforms using the GENET driver.
>
> First patch fixes an issue with the PHY state machine queuing work after
> resume since we are not properly detached from it.
>
> Second patch fixes an issue with GENET interfaces that were not properly
> restored to a working state after a S3 suspend/resume cycle.
Series applied, thanks.
^ permalink raw reply
* [PATCH net-next] hyperv: Add processing of MTU reduced by the host
From: Haiyang Zhang @ 2014-11-11 23:27 UTC (permalink / raw)
To: davem, netdev; +Cc: olaf, jasowang, driverdev-devel, linux-kernel, haiyangz
If the host uses packet encapsulation feature, the MTU may be reduced by the
host due to headroom reservation for encapsulation. This patch handles this
new MTU value.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/netvsc.c | 3 ++-
drivers/net/hyperv/netvsc_drv.c | 4 ++--
drivers/net/hyperv/rndis_filter.c | 9 +++++++++
3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 7d76c95..6b46311 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -440,7 +440,8 @@ static int negotiate_nvsp_ver(struct hv_device *device,
/* NVSPv2 only: Send NDIS config */
memset(init_packet, 0, sizeof(struct nvsp_message));
init_packet->hdr.msg_type = NVSP_MSG2_TYPE_SEND_NDIS_CONFIG;
- init_packet->msg.v2_msg.send_ndis_config.mtu = net_device->ndev->mtu;
+ init_packet->msg.v2_msg.send_ndis_config.mtu = net_device->ndev->mtu +
+ ETH_HLEN;
init_packet->msg.v2_msg.send_ndis_config.capability.ieee8021q = 1;
ret = vmbus_sendpacket(device->channel, init_packet,
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 3295e4e..b689f96 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -699,9 +699,9 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
return -ENODEV;
if (nvdev->nvsp_version >= NVSP_PROTOCOL_VERSION_2)
- limit = NETVSC_MTU;
+ limit = NETVSC_MTU - ETH_HLEN;
- if (mtu < 68 || mtu > limit)
+ if (mtu < ETH_DATA_LEN || mtu > limit)
return -EINVAL;
nvdev->start_remove = true;
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index ccce6f2..7b2c5d1 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -998,6 +998,7 @@ int rndis_filter_device_add(struct hv_device *dev,
int t;
struct ndis_recv_scale_cap rsscap;
u32 rsscap_size = sizeof(struct ndis_recv_scale_cap);
+ u32 mtu, size;
rndis_device = get_rndis_device();
if (!rndis_device)
@@ -1029,6 +1030,14 @@ int rndis_filter_device_add(struct hv_device *dev,
return ret;
}
+ /* Get the MTU from the host */
+ size = sizeof(u32);
+ ret = rndis_filter_query_device(rndis_device,
+ RNDIS_OID_GEN_MAXIMUM_FRAME_SIZE,
+ &mtu, &size);
+ if (ret == 0 && size == sizeof(u32))
+ net_device->ndev->mtu = mtu;
+
/* Get the mac address */
ret = rndis_filter_query_device_mac(rndis_device);
if (ret != 0) {
--
1.7.1
^ permalink raw reply related
* Re: [PATCH net-next 1/1] ipvlan: Initial check-in of the IPVLAN driver.
From: Eric Dumazet @ 2014-11-11 23:28 UTC (permalink / raw)
To: Mahesh Bandewar
Cc: netdev, Eric Dumazet, Maciej Zenczykowski, Laurent Chavey,
Tim Hockin, David Miller, Brandon Philips, Pavel Emelianov
In-Reply-To: <1415744984-25802-1-git-send-email-maheshb@google.com>
On Tue, 2014-11-11 at 14:29 -0800, Mahesh Bandewar wrote:
...
> +static void *ipvlan_get_L3_hdr(struct sk_buff *skb, int *type)
> +{
> + void *lyr3h = NULL;
> +
> + switch (skb->protocol) {
> + case htons(ETH_P_ARP): {
> + struct arphdr *arph;
> +
> + if (unlikely(!pskb_may_pull(skb, sizeof(struct arphdr))))
> + return NULL;
> +
> + arph = arp_hdr(skb);
> + *type = IPVL_ARP;
> + lyr3h = arph;
> + break;
> + }
> +
> + case htons(ETH_P_IP): {
> + u32 pktlen;
> + struct iphdr *ip4h;
> +
> + if (unlikely(!pskb_may_pull(skb, sizeof(struct iphdr))))
> + return NULL;
> +
> + ip4h = ip_hdr(skb);
> + pktlen = ntohs(ip4h->tot_len);
> + if (ip4h->ihl < 5 || ip4h->version != 4)
> + return NULL;
> + if (skb->len < pktlen || pktlen < (ip4h->ihl * 4))
> + return NULL;
> +
> + *type = IPVL_IPV4;
> + lyr3h = ip4h;
> + break;
> + }
> + case htons(ETH_P_IPV6): {
> + struct ipv6hdr *ip6h;
> +
> + if (unlikely(!pskb_may_pull(skb, sizeof(struct iphdr))))
sizeof(struct ipv6hdr) or sizeof(*ip6h)
> + return NULL;
> +
> + ip6h = ipv6_hdr(skb);
> + if (ip6h->version != 6)
> + return NULL;
> +
> + *type = IPVL_IPV6;
> + lyr3h = ip6h;
> + /* Only Neighbour Solicitation pkts need different treatment */
> + if (ipv6_addr_any(&ip6h->saddr) &&
> + ip6h->nexthdr == NEXTHDR_ICMP) {
> + /* Get to the ICMPv6 header */
> + *type = IPVL_ICMPV6;
> + lyr3h = ip6h + 1;
> + }
> + break;
> + }
> + default:
> + return NULL;
> + }
> +
> + return lyr3h;
> +}
...
> +static int ipvlan_process_v6_outbound(struct sk_buff *skb)
> +{
> + const struct ipv6hdr *ip6h = ipv6_hdr(skb);
> + struct net_device *dev = skb->dev;
> + struct dst_entry *dst;
> + int err, ret = NET_XMIT_DROP;
> + struct flowi6 fl6 = {
> + .flowi6_iif = skb->dev->ifindex,
> + .daddr = ip6h->daddr,
> + .saddr = ip6h->saddr,
> + .flowi6_flags = FLOWI_FLAG_ANYSRC,
> + .flowlabel = ip6_flowinfo(ip6h),
> + .flowi6_mark = skb->mark,
> + .flowi6_proto = ip6h->nexthdr,
> + };
> +
> + dst = ip6_route_output(dev_net(dev), NULL, &fl6);
> + if (IS_ERR(dst)) {
> + err = PTR_ERR(dst);
> + dst = NULL;
dst = NULL; seems not needed.
> + goto err;
> + }
> + skb_dst_drop(skb);
> + skb_dst_set(skb, dst);
> + err = ip6_local_out(skb);
> + if (unlikely(net_xmit_eval(err)))
> + dev->stats.tx_errors++;
> + else
> + ret = NET_XMIT_SUCCESS;
> + goto out;
> +err:
> + dev->stats.tx_errors++;
> + kfree_skb(skb);
> +out:
> + return ret;
> +}
...
> +static rx_handler_result_t ipvlan_handle_mode_l2(struct sk_buff **pskb,
> + struct ipvl_port *port)
> +{
> + struct sk_buff *skb = *pskb;
> + struct ethhdr *eth = eth_hdr(skb);
> + rx_handler_result_t ret = RX_HANDLER_PASS;
> + void *lyr3h;
> + int addr_type;
> +
> + /* First Handle multi-cast frames */
> + if (is_multicast_ether_addr(eth->h_dest)) {
> + /* Pass to virtual devs only if they haven't seen the frame. */
> + if (ipvlan_external_frame(skb, port)) {
> + ipvlan_dbg(4, "%s[%d]L2:Mcast Recv:[%s], PROT=[%x]\n",
> + __func__, __LINE__, port->dev->name,
> + ntohs(skb->protocol));
> + ipvlan_multicast_frame(port, skb, NULL, false);
> + }
> + } else if ((lyr3h = ipvlan_get_L3_hdr(skb, &addr_type)) != NULL) {
> + struct ipvl_addr *addr = NULL;
= NULL; not needed.
> +
> + addr = ipvlan_addr_lookup(port, lyr3h, addr_type, true);
> + if (addr) {
> + ipvlan_dbg(4, "%s[%d]L2:Ucast Recv:[%s], PROT=[%x]\n",
> + __func__, __LINE__, addr->master->dev->name,
> + ntohs(skb->protocol));
> + ret = ipvlan_rcv_frame(addr, skb, false);
> + }
> + }
> +
> + return ret;
> +}
> +
> +rx_handler_result_t ipvlan_handle_frame(struct sk_buff **pskb)
> +{
> + struct sk_buff *skb = *pskb;
> + struct ipvl_port *port = ipvlan_port_get_rcu(skb->dev);
> +
> + if (!port)
> + goto out;
> +
> + if (unlikely(!pskb_may_pull(skb, sizeof(struct ethhdr))))
This looks strange.
Here we are sure ethernet header was already pulled by eth_type_trans()
> + goto out;
> +
> + switch (port->mode) {
> + case IPVLAN_MODE_L2:
> + return ipvlan_handle_mode_l2(pskb, port);
> + case IPVLAN_MODE_L3:
> + return ipvlan_handle_mode_l3(pskb, port);
> + }
> +
> + /* Should not reach here */
> + BUG();
> +out:
> + return RX_HANDLER_PASS;
> +}
> diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
> new file mode 100644
> index 000000000000..e87b6eb01060
> --- /dev/null
> +++ b/drivers/net/ipvlan/ipvlan_main.c
> @@ -0,0 +1,828 @@
> +/* Copyright (c) 2014 Mahesh Bandewar <maheshb@google.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + */
> +
> +
...
> +static int ipvlan_add_addr6(struct ipvl_dev *ipvlan, struct in6_addr *ip6_addr)
> +{
> + struct ipvl_addr *addr = NULL;
> +
> + if (ipvlan_addr_busy(ipvlan, ip6_addr, true)) {
> + pr_warn("%s[%d]: Failed IPv6=%x:%x:%x:%x address for %s intf\n",
> + __func__, __LINE__, ip6_addr->s6_addr32[0],
> + ip6_addr->s6_addr32[1], ip6_addr->s6_addr32[2],
> + ip6_addr->s6_addr32[3], ipvlan->dev->name);
> + return -EINVAL;
> + }
> + if ((addr = kzalloc(sizeof(struct ipvl_addr), GFP_ATOMIC)) == NULL)
Why is GFP_ATOMIC used here ?
> + return -ENOMEM;
> +
> + ipvlan_dbg(1, "%s[%d]: Adding IPv6=%x:%x:%x:%x address for %s intf\n",
> + __func__, __LINE__, ip6_addr->s6_addr32[0],
> + ip6_addr->s6_addr32[1], ip6_addr->s6_addr32[2],
> + ip6_addr->s6_addr32[3], ipvlan->dev->name);
> + addr->master = ipvlan;
> + memcpy(&addr->ip6addr, ip6_addr, sizeof(struct in6_addr));
> + addr->atype = IPVL_IPV6;
> + list_add_tail_rcu(&addr->anode, &ipvlan->addrs);
> + ipvlan->ipv6cnt++;
> + ipvlan_ht_addr_add(ipvlan, addr);
> +
> + return 0;
> +}
> +
> +static void ipvlan_del_addr6(struct ipvl_dev *ipvlan, struct in6_addr *ip6_addr)
> +{
> + struct ipvl_addr *addr = NULL;
> +
> + if ((addr = ipvlan_ht_addr_lookup(ipvlan->port, ip6_addr, true)) ==NULL)
> + return;
> +
> + ipvlan_dbg(1,
> + "%s[%d]: Deleting IPv6=%x:%x:%x:%x address for %s intf.\n",
> + __func__, __LINE__, ip6_addr->s6_addr32[0],
> + ip6_addr->s6_addr32[1], ip6_addr->s6_addr32[2],
> + ip6_addr->s6_addr32[3], ipvlan->dev->name);
> + /* Delete from the hash-table */
> + ipvlan_ht_addr_del(addr, true);
> + /* Delete from the logical's addr list */
> + list_del_rcu(&addr->anode);
> + ipvlan->ipv6cnt--;
> + WARN_ON(ipvlan->ipv6cnt < 0);
> + kfree_rcu(addr, rcu);
> +
> + return;
> +}
> +
> +static int ipvlan_addr6_event(struct notifier_block *unused,
> + unsigned long event, void *ptr)
> +{
> + struct inet6_ifaddr *if6 = (struct inet6_ifaddr *)ptr;
> + struct net_device *dev = (struct net_device *)if6->idev->dev;
> + struct ipvl_dev *ipvlan = netdev_priv(dev);
> +
> + ipvlan_dbg(3, "%s[%d]: Entering...\n", __func__, __LINE__);
> + if (!ipvlan_dev_slave(dev))
> + return NOTIFY_DONE;
> +
> + if (!ipvlan || !ipvlan->port)
> + return NOTIFY_DONE;
> +
> + switch (event) {
> + case NETDEV_UP:
> + if (ipvlan_add_addr6(ipvlan, &if6->addr))
> + return NOTIFY_BAD;
> + break;
> +
> + case NETDEV_DOWN:
> + ipvlan_del_addr6(ipvlan, &if6->addr);
> + break;
> + }
> +
> + ipvlan_dbg(3, "%s[%d]: Leaving...\n", __func__, __LINE__);
> + return NOTIFY_OK;
> +}
> +
> +static int ipvlan_add_addr4(struct ipvl_dev *ipvlan, struct in_addr *ip4_addr)
> +{
> + struct ipvl_addr *addr = NULL;
> +
> + if (ipvlan_addr_busy(ipvlan, ip4_addr, false)) {
> + pr_warn("%s[%d]: Failed to add IPv4=%x on %s intf.\n",
> + __func__, __LINE__, ntohl(ip4_addr->s_addr),
> + ipvlan->dev->name);
> + return -EINVAL;
> + }
> + if ((addr = kzalloc(sizeof(struct ipvl_addr), GFP_ATOMIC)) == NULL)
Same issue here ? GFP_KERNEL should be OK.
^ permalink raw reply
* Re: [PATCH] smsc911x: power-up phydev before doing a software reset.
From: David Miller @ 2014-11-11 23:30 UTC (permalink / raw)
To: eballetbo; +Cc: netdev, steve.glendinning, javier, ebutera
In-Reply-To: <1415643789-5367-1-git-send-email-eballetbo@iseebcn.com>
From: Enric Balletbo i Serra <eballetbo@iseebcn.com>
Date: Mon, 10 Nov 2014 19:23:09 +0100
> With commit be9dad1f9f26604fb ("net: phy: suspend phydev when going
> to HALTED"), the PHY device will be put in a low-power mode using
> BMCR_PDOWN if the the interface is set down. The smsc911x driver does
> a software_reset opening the device driver (ndo_open). In such case,
> the PHY must be powered-up before access to any register and before
> calling the software_reset function. Otherwise, as the PHY is powered
> down the software reset fails and the interface can not be enabled
> again.
>
> This patch fixes this scenario that is easy to reproduce setting down
> the network interface and setting up again.
>
> $ ifconfig eth0 down
> $ ifconfig eth0 up
> ifconfig: SIOCSIFFLAGS: Input/output error
>
> Signed-off-by: Enric Balletbo i Serra <eballetbo@iseebcn.com>
...
> + mdelay(1);
As per Javier's feedback, please convert this to usleep_range()
if you agree that this can only be invoked from process context.
Thanks.
^ permalink raw reply
* Re: "asix: Don't reset PHY on if_up for ASIX 88772" breaks net on arndale platform
From: Ben Hutchings @ 2014-11-12 0:23 UTC (permalink / raw)
To: Charles Keepax
Cc: Stam, Michel [FINT], Riku Voipio, davem, linux-usb, netdev,
linux-kernel, linux-samsung-soc
In-Reply-To: <20141104200914.GN23178@opensource.wolfsonmicro.com>
[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]
On Tue, 2014-11-04 at 20:09 +0000, Charles Keepax wrote:
> On Tue, Nov 04, 2014 at 11:23:06AM +0100, Stam, Michel [FINT] wrote:
> > Hello Riku,
> >
> > >Fixing a bug (ethtool support) must not cause breakage elsewhere (in
> > this case on arndale). This is now a regression of functionality from
> > 3.17.
> > >
> > >I think it would better to revert the change now and with less hurry
> > introduce a ethtool fix that doesn't break arndale.
> >
> > I don't fully agree here;
> > I would like to point out that this commit is a revert itself. Fixing
> > the armdale will then cause breakage in other implementations, such as
> > ours. Blankly reverting breaks other peoples' implementations.
> >
> > The PHY reset is the thing that breaks ethtool support, so any fix that
> > appeases all would have to take existing PHY state into account.
[...]
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -299,6 +299,7 @@ static int ax88772_reset(struct usbnet *dev)
> {
> struct asix_data *data = (struct asix_data *)&dev->data;
> int ret, embd_phy;
> + int reg;
> u16 rx_ctl;
>
> ret = asix_write_gpio(dev,
> @@ -359,8 +360,10 @@ static int ax88772_reset(struct usbnet *dev)
> msleep(150);
>
> asix_mdio_write(dev->net, dev->mii.phy_id, MII_BMCR, BMCR_RESET);
> - asix_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE,
> - ADVERTISE_ALL | ADVERTISE_CSMA);
> + reg = asix_mdio_read(dev->net, dev->mii.phy_id, MII_ADVERTISE);
> + if (!reg)
> + reg = ADVERTISE_ALL | ADVERTISE_CSMA;
> + asix_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE, reg);
[...]
Why is there no sleep after setting the RESET bit? Doesn't that make
the following register writes unreliable?
Ben.
--
Ben Hutchings
Experience is directly proportional to the value of equipment destroyed.
- Carolyn Scheppner
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 1/1] ipvlan: Initial check-in of the IPVLAN driver.
From: Cong Wang @ 2014-11-12 0:37 UTC (permalink / raw)
To: David Miller
Cc: Mahesh Bandewar, netdev, Eric Dumazet, Maciej Żenczykowski,
Laurent Chavey, Tim Hockin, Brandon Philips, Pavel Emelyanov
In-Reply-To: <20141111.181900.1713364166494134373.davem@davemloft.net>
On Tue, Nov 11, 2014 at 3:19 PM, David Miller <davem@davemloft.net> wrote:
> From: Cong Wang <cwang@twopensource.com>
> Date: Tue, 11 Nov 2014 15:12:27 -0800
>
>> On Tue, Nov 11, 2014 at 2:29 PM, Mahesh Bandewar <maheshb@google.com> wrote:
>>> This driver is very similar to the macvlan driver except that it
>>> uses L3 on the frame to determine the logical interface while
>>> functioning as packet dispatcher. It inherits L2 of the master
>>> device hence the packets on wire will have the same L2 for all
>>> the packets originating from all virtual devices off of the same
>>> master device.
>>
>> Why do we need this from the beginning?
>> IOW, what problem does this solve while macvlan doesn't?
>
> macvlan has several built-in limitations, which IP VLAN absolutely
> does not have.
>
> Eric Dumazet spoke about this at the networking track at the kernel
> summit in Chicago, maybe he or another person working on this can
> chime in.
Either you need to publish it or document it in this changelog,
otherwise too much information is missing.
^ permalink raw reply
* Re: [PATCH net-next 1/1] ipvlan: Initial check-in of the IPVLAN driver.
From: Cong Wang @ 2014-11-12 0:39 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Mahesh Bandewar, netdev, Eric Dumazet, Maciej Zenczykowski,
Laurent Chavey, Tim Hockin, David Miller, Brandon Philips,
Pavel Emelianov
In-Reply-To: <1415748160.2292.8.camel@localhost>
On Tue, Nov 11, 2014 at 3:22 PM, Hannes Frederic Sowa <hannes@redhat.com> wrote:
> On Di, 2014-11-11 at 15:12 -0800, Cong Wang wrote:
>> On Tue, Nov 11, 2014 at 2:29 PM, Mahesh Bandewar <maheshb@google.com> wrote:
>> > This driver is very similar to the macvlan driver except that it
>> > uses L3 on the frame to determine the logical interface while
>> > functioning as packet dispatcher. It inherits L2 of the master
>> > device hence the packets on wire will have the same L2 for all
>> > the packets originating from all virtual devices off of the same
>> > master device.
>>
>> Why do we need this from the beginning?
>> IOW, what problem does this solve while macvlan doesn't?
>
> I think it is good to reduce the number of mac addresses before a NIC
> switches into promisc mode.
>
Sounds like over-kill to have a new device just for not worrying about mac.
Or you mean our neigh table doesn't scale?
^ permalink raw reply
* Re: [PATCH] brcmfmac: unlink URB when request timed out
From: Mathy Vanhoef @ 2014-11-12 0:40 UTC (permalink / raw)
To: Arend van Spriel, brudley, frankyl, meuleman, linville, pieterpg,
linux-wireless, brcm80211-dev-list, netdev, Oliver Neukum
In-Reply-To: <54624EB4.5060501@broadcom.com>
On 11/11/2014 01:00 PM, Arend van Spriel wrote:
> On 11-11-14 18:35, Mathy Vanhoef wrote:
>> Using usb_kill_urb() instead of usb_unlink_urb() seems to work without any problems.
>
> Ok, as usb_kill_urb() assures the completion handler is called there is
> no need to check the ctl_completed flag, ie. my patch. Go ahead and
> resubmit your patch replacing usb_unlink_urb() by usb_kill_urb().
>
> Do you mean you end up with a working usb device providing a wireless
> interface? Or does probe still fail, but you do not get a crash.
I end up with a working device which can connect to networks.
Kind regards,
Mathy
> Regards,
> Arend
>
>> On 11/11/2014 06:05 AM, Arend van Spriel wrote:
>>> On 10-11-14 17:08, Mathy Vanhoef wrote:
>>>> On 11/10/2014 06:18 AM, Arend van Spriel wrote:
>>>>> On 09-11-14 19:10, Mathy Vanhoef wrote:
>>>>>> From: Mathy Vanhoef <vanhoefm@gmail.com>
>>>>>>
>>>>>> Unlink the submitted URB in brcmf_usb_dl_cmd if the request timed out. This
>>>>>> assures the URB is never submitted twice, preventing a driver crash.
>>>>>
>>>>> Hi Mathy,
>>>>>
>>>>> What driver crash are you referring to? The log only shows the WARNING
>>>>> ending in a USB disconnect but no actual crash. Does your patch get the
>>>>> driver running properly or does it only avoid the warning.
>>>>
>>>> Hi Arend,
>>>>
>>>> It shows a warning, after which the device doesn't work (but the computer is
>>>> still usable). But I've noticed that when *unplugging* the USB cable the OS may
>>>> freeze. This doesn't always happen though, sometimes unplugging works OK. The
>>>> patch both avoids the warning, and gets the device/driver running properly
>>>> (unplugging also works OK).
>>>>
>>>>>
>>>>> With that said, it seems there is some need for improvement, but I also
>>>>> notice you are running this on a virtual machine so could that affect
>>>>> the timeout to kick in before completion. Could you try to increase the
>>>>> timeout. Still when a timeout occurs this needs to be handled properly.
>>>>> Could you also try the following patch?
>>>>
>>>> I did a few additional tests:
>>>>
>>>> 1. When increasing IOCTL_RESP_TIMEOUT to 20000 (ten times the normal value) the
>>>> timeout and warning still occur. Device/driver doesn't work.
>>>> 2. When increasing BRCMF_USB_RESET_GETVER_SPINWAIT to 1000 (ten timers the
>>>> normal value) everything works. Device/driver works.
>>>
>>> This means the ctl_urb completes on your system within 3sec, but not
>>> within 2.1sec. After discussing this with my colleague, we think you
>>> should use usb_kill_urb() instead of usb_unlink_urb() as it assures the
>>> completion handler is called. Could you retest that and let us know.
>>>
>>> Regards,
>>> Arend
>>>
>>>> 3. Quick test using backports-3.18-rc1-1 (aka unpatched driver) on a non-
>>>> virtualized Linux install: In that case everything worked fine. So the bug
>>>> may only be triggered in a virtualized environment / VMWare.
>>>> 4. When applying your patch, the driver stops early during initialization of
>>>> the device. I included a WARN_ONCE before returning EINVAL and got the
>>>> output below.
>>>>
>>>> Kind regards,
>>>> Mathy
>>>> ---
>>>> [ 220.955647] usb 1-1: new high-speed USB device number 3 using ehci-pci
>>>> [ 221.487797] usb 1-1: New USB device found, idVendor=043e, idProduct=3004
>>>> [ 221.487802] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
>>>> [ 221.487804] usb 1-1: Product: Remote Download Wireless Adapter
>>>> [ 221.487806] usb 1-1: Manufacturer: Broadcom
>>>> [ 221.487808] usb 1-1: SerialNumber: 000000000001
>>>> [ 221.490472] brcmfmac: brcmf_usb_probe Enter 0x043e:0x3004
>>>> [ 221.490476] brcmfmac: brcmf_usb_probe Broadcom high speed USB WLAN interface detected
>>>> [ 221.490477] brcmfmac: brcmf_usb_probe_cb Enter
>>>> [ 221.490480] brcmfmac: brcmf_usb_attach Enter
>>>> [ 221.490494] brcmfmac: brcmf_usb_dlneeded Enter
>>>> [ 221.490495] ------------[ cut here ]------------
>>>> [ 221.490503] WARNING: CPU: 0 PID: 100 at drivers/net/wireless/brcm80211/brcmfmac/usb.c:716 brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]()
>>>> [ 221.490505] EINVAL devinfo=c0044000 ctl_rub=ef898380 completed=0
>>>> [ 221.490506] Modules linked in: brcmfmac brcmutil vmw_pvscsi pcnet32 mptspi mptscsih mptbase
>>>> [ 221.490514] CPU: 0 PID: 100 Comm: kworker/0:1 Not tainted 3.18.0-rc3-wl+ #2
>>>> [ 221.490515] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/31/2013
>>>> [ 221.490528] Workqueue: usb_hub_wq hub_event
>>>> [ 221.490530] 00000000 00000000 eecffb58 c1711f4a eecffb98 eecffb88 c103edaf f11cbc58
>>>> [ 221.490534] eecffbb4 00000064 f11cbc84 000002cc f11c1595 f11c1595 c0044000 ffffffea
>>>> [ 221.490537] ef726000 eecffba0 c103ee4e 00000009 eecffb98 f11cbc58 eecffbb4 eecffbd0
>>>> [ 221.490541] Call Trace:
>>>> [ 221.490550] [<c1711f4a>] dump_stack+0x41/0x52
>>>> [ 221.490558] [<c103edaf>] warn_slowpath_common+0x7f/0xa0
>>>> [ 221.490563] [<f11c1595>] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]
>>>> [ 221.490567] [<f11c1595>] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]
>>>> [ 221.490570] [<c103ee4e>] warn_slowpath_fmt+0x2e/0x30
>>>> [ 221.490575] [<f11c1595>] brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]
>>>> [ 221.490580] [<f11c2cd8>] brcmf_usb_probe+0x3c8/0x640 [brcmfmac]
>>>> [ 221.490583] [<c1717d53>] ? mutex_lock+0x13/0x32
>>>> [ 221.490586] [<c1493ae3>] usb_probe_interface+0xa3/0x180
>>>> [ 221.490590] [<c13f5690>] ? __driver_attach+0x90/0x90
>>>> [ 221.490592] [<c13f546e>] driver_probe_device+0x5e/0x1f0
>>>> [ 221.490595] [<c13f5690>] ? __driver_attach+0x90/0x90
>>>> [ 221.490597] [<c13f56c9>] __device_attach+0x39/0x50
>>>> [ 221.490600] [<c13f3d84>] bus_for_each_drv+0x34/0x70
>>>> [ 221.490602] [<c13f53db>] device_attach+0x7b/0x90
>>>> [ 221.490604] [<c13f5690>] ? __driver_attach+0x90/0x90
>>>> [ 221.490607] [<c13f4b8f>] bus_probe_device+0x6f/0x90
>>>> [ 221.490609] [<c13f3256>] device_add+0x426/0x520
>>>> [ 221.490611] [<c1491503>] ? usb_control_msg+0xb3/0xd0
>>>> [ 221.490614] [<c1717d53>] ? mutex_lock+0x13/0x32
>>>> [ 221.490627] [<c14922f8>] usb_set_configuration+0x3f8/0x700
>>>> [ 221.490630] [<c13f5690>] ? __driver_attach+0x90/0x90
>>>> [ 221.490633] [<c149ac7b>] generic_probe+0x2b/0x90
>>>> [ 221.490637] [<c1188bc0>] ? sysfs_create_link+0x20/0x40
>>>> [ 221.490639] [<c1492bec>] usb_probe_device+0xc/0x10
>>>> [ 221.490641] [<c13f546e>] driver_probe_device+0x5e/0x1f0
>>>> [ 221.490644] [<c13f5690>] ? __driver_attach+0x90/0x90
>>>> [ 221.490646] [<c13f56c9>] __device_attach+0x39/0x50
>>>> [ 221.490649] [<c13f3d84>] bus_for_each_drv+0x34/0x70
>>>> [ 221.490651] [<c13f53db>] device_attach+0x7b/0x90
>>>> [ 221.490653] [<c13f5690>] ? __driver_attach+0x90/0x90
>>>> [ 221.490656] [<c13f4b8f>] bus_probe_device+0x6f/0x90
>>>> [ 221.490658] [<c13f3256>] device_add+0x426/0x520
>>>> [ 221.490661] [<c148aa2e>] ? usb_new_device+0x16e/0x3a0
>>>> [ 221.490663] [<c148aad7>] usb_new_device+0x217/0x3a0
>>>> [ 221.490666] [<c148bff7>] hub_event+0xa17/0xda0
>>>> [ 221.490668] [<c1716918>] ? __schedule+0x2f8/0x710
>>>> [ 221.490672] [<c105127c>] ? pwq_dec_nr_in_flight+0x3c/0x90
>>>> [ 221.490674] [<c10513ee>] process_one_work+0x11e/0x360
>>>> [ 221.490677] [<c1051750>] worker_thread+0xf0/0x3c0
>>>> [ 221.490680] [<c106e14a>] ? __wake_up_locked+0x1a/0x20
>>>> [ 221.490682] [<c1051660>] ? process_scheduled_works+0x30/0x30
>>>> [ 221.490685] [<c1055b56>] kthread+0x96/0xb0
>>>> [ 221.490687] [<c1050000>] ? put_unbound_pool+0x110/0x170
>>>> [ 221.490691] [<c1719c81>] ret_from_kernel_thread+0x21/0x30
>>>> [ 221.490693] [<c1055ac0>] ? kthread_worker_fn+0x110/0x110
>>>> [ 221.490695] ---[ end trace 9befd914693f3083 ]---
>>>> [ 221.490697] brcmfmac: brcmf_usb_dlneeded chip 57005 rev 0xf11cfcec
>>>> [ 221.490699] brcmfmac: brcmf_fw_get_firmwares enter: dev=1-1
>>>>
>>>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>>>> index 5265aa7..15b1aa7 100644
>>>> --- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>>>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>>>> @@ -709,8 +709,13 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info *devinfo, u8 cmd,
>>>> char *tmpbuf;
>>>> u16 size;
>>>>
>>>> - if ((!devinfo) || (devinfo->ctl_urb == NULL))
>>>> + if (!devinfo || !devinfo->ctl_urb || !devinfo->ctl_completed) {
>>>> + WARN_ONCE(1, "EINVAL devinfo=%p ctl_rub=%p completed=%d\n",
>>>> + devinfo,
>>>> + devinfo ? devinfo->ctl_urb : NULL,
>>>> + devinfo ? devinfo->ctl_completed : -1);
>>>> return -EINVAL;
>>>> + }
>>>>
>>>> tmpbuf = kmalloc(buflen, GFP_ATOMIC);
>>>> if (!tmpbuf)
>>>>
>>>>
>>>>>
>>>>> Regards,
>>>>> Arend
>>>>> ---
>>>>> drivers/net/wireless/brcm80211/brcmfmac/usb.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>>>>> b/drivers/net/wireles
>>>>> index dc13591..786c40b 100644
>>>>> --- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>>>>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>>>>> @@ -640,7 +640,7 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info
>>>>> *devinf
>>>>> char *tmpbuf;
>>>>> u16 size;
>>>>>
>>>>> - if ((!devinfo) || (devinfo->ctl_urb == NULL))
>>>>> + if (!devinfo || !devinfo->ctl_urb || !devinfo->ctl_completed)
>>>>> return -EINVAL;
>>>>>
>>>>> tmpbuf = kmalloc(buflen, GFP_ATOMIC);
>>>>>
>>>>>> Signed-off-by: Mathy Vanhoef <vanhoefm@gmail.com>
>>>>>> ---
>>>>>> Currently brcmfmac may crash when a USB device is attached (tested with a LG
>>>>>> TWFM-B003D). In particular it fails on the second call to brcmf_usb_dl_cmd in
>>>>>> the while loop of brcmf_usb_resetcfg. The problem is that an URB is being
>>>>>> submitted twice:
>>>>>>
>>>>>> [ 169.861800] brcmfmac: brcmf_usb_dl_writeimage Enter, fw f14db000, len 348160
>>>>>> [ 171.787791] brcmfmac: brcmf_usb_dl_writeimage Exit, err=0
>>>>>> [ 171.787797] brcmfmac: brcmf_usb_dlstart Exit, err=0
>>>>>> [ 171.787799] brcmfmac: brcmf_usb_dlrun Enter
>>>>>> [ 171.791794] brcmfmac: brcmf_usb_resetcfg Enter
>>>>>> [ 173.988072] ------------[ cut here ]------------
>>>>>> [ 173.988083] WARNING: CPU: 0 PID: 369 at drivers/usb/core/urb.c:339 usb_submit_urb+0x4e6/0x500()
>>>>>> [ 173.988085] URB eaf45f00 submitted while active
>>>>>> [ 173.988086] Modules linked in: brcmfmac brcmutil vmw_pvscsi pcnet32 mptspi mptscsih mptbase
>>>>>> [ 173.988100] CPU: 0 PID: 369 Comm: kworker/0:2 Not tainted 3.18.0-rc3-wl #1
>>>>>> [ 173.988102] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/31/2013
>>>>>> [ 173.988106] Workqueue: events request_firmware_work_func
>>>>>> [ 173.988108] 00000000 00000000 ee747db8 c1711f4a ee747df8 ee747de8 c103edaf c18d1e10
>>>>>> [ 173.988112] ee747e14 00000171 c18a8b29 00000153 c1490556 c1490556 eaf45f00 eafdc660
>>>>>> [ 173.988115] f14b8fa0 ee747e00 c103ee4e 00000009 ee747df8 c18d1e10 ee747e14 ee747e50
>>>>>> [ 173.988119] Call Trace:
>>>>>> [ 173.988129] [<c1711f4a>] dump_stack+0x41/0x52
>>>>>> [ 173.988136] [<c103edaf>] warn_slowpath_common+0x7f/0xa0
>>>>>> [ 173.988139] [<c1490556>] ? usb_submit_urb+0x4e6/0x500
>>>>>> [ 173.988141] [<c1490556>] ? usb_submit_urb+0x4e6/0x500
>>>>>> [ 173.988147] [<f14b8fa0>] ? brcmf_usb_ioctl_resp_wake+0x40/0x40 [brcmfmac]
>>>>>> [ 173.988150] [<c103ee4e>] warn_slowpath_fmt+0x2e/0x30
>>>>>> [ 173.988152] [<c1490556>] usb_submit_urb+0x4e6/0x500
>>>>>> [ 173.988156] [<c1123de1>] ? __kmalloc+0x21/0x140
>>>>>> [ 173.988161] [<f14b91c3>] ? brcmf_usb_dl_cmd+0x33/0x120 [brcmfmac]
>>>>>> [ 173.988166] [<f14b9243>] brcmf_usb_dl_cmd+0xb3/0x120 [brcmfmac]
>>>>>> [ 173.988170] [<f14ba6c4>] brcmf_usb_probe_phase2+0x4e4/0x640 [brcmfmac]
>>>>>> [ 173.988176] [<f14b4900>] brcmf_fw_request_code_done+0xd0/0xf0 [brcmfmac]
>>>>>> [ 173.988178] [<c1400876>] request_firmware_work_func+0x26/0x50
>>>>>> [ 173.988182] [<c10513ee>] process_one_work+0x11e/0x360
>>>>>> [ 173.988184] [<c1051750>] worker_thread+0xf0/0x3c0
>>>>>> [ 173.988205] [<c106e14a>] ? __wake_up_locked+0x1a/0x20
>>>>>> [ 173.988208] [<c1051660>] ? process_scheduled_works+0x30/0x30
>>>>>> [ 173.988211] [<c1055b56>] kthread+0x96/0xb0
>>>>>> [ 173.988214] [<c1719c81>] ret_from_kernel_thread+0x21/0x30
>>>>>> [ 173.988217] [<c1055ac0>] ? kthread_worker_fn+0x110/0x110
>>>>>> [ 173.988219] ---[ end trace 0c88bf46801de083 ]---
>>>>>> [ 173.988221] brcmf_usb_dl_cmd: usb_submit_urb failed -16
>>>>>> [ 173.988396] brcmfmac: brcmf_usb_probe_phase2 failed: dev=1-1, err=-19
>>>>>> [ 173.989503] brcmfmac: brcmf_usb_disconnect Enter
>>>>>>
>>>>>> This patch fixes the brcmf_usb_dl_cmd function to prevent an URB from being
>>>>>> submitted twice. Tested using a LG TWFM-B003D, which now works properly.
>>>>>>
>>>>>>
>>>>>> drivers/net/wireless/brcm80211/brcmfmac/usb.c | 6 ++++--
>>>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>>>>>> index 5265aa7..1bc7858 100644
>>>>>> --- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>>>>>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>>>>>> @@ -738,10 +738,12 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info *devinfo, u8 cmd,
>>>>>> goto finalize;
>>>>>> }
>>>>>>
>>>>>> - if (!brcmf_usb_ioctl_resp_wait(devinfo))
>>>>>> + if (!brcmf_usb_ioctl_resp_wait(devinfo)) {
>>>>>> + usb_unlink_urb(devinfo->ctl_urb);
>>>>>> ret = -ETIMEDOUT;
>>>>>> - else
>>>>>> + } else {
>>>>>> memcpy(buffer, tmpbuf, buflen);
>>>>>> + }
>>>>>>
>>>>>> finalize:
>>>>>> kfree(tmpbuf);
>>>>>>
>>>>>
>>>
>
^ permalink raw reply
* Re: [PATCH] brcmfmac: unlink URB when request timed out
From: Mathy Vanhoef @ 2014-11-12 1:02 UTC (permalink / raw)
To: Oliver Neukum
Cc: brudley, Arend van Spriel, Franky Lin, meuleman, John Linville,
pieterpg, linux-wireless, brcm80211-dev-list, netdev,
linux-kernel
In-Reply-To: <1415610506.16488.20.camel@linux-0dmf.site>
On 11/10/2014 04:08 AM, Oliver Neukum wrote:
> On Sun, 2014-11-09 at 13:10 -0500, Mathy Vanhoef wrote:
>> From: Mathy Vanhoef <vanhoefm@gmail.com>
>>
>> Unlink the submitted URB in brcmf_usb_dl_cmd if the request timed out. This
>> assures the URB is never submitted twice, preventing a driver crash.
>
> Hi,
>
> I am afrad this patch is no good. The diagnosis is good,
> but the fix introduces serious problems.
>
>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>> index 5265aa7..1bc7858 100644
>> --- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>> @@ -738,10 +738,12 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info *devinfo, u8 cmd,
>> goto finalize;
>> }
>>
>> - if (!brcmf_usb_ioctl_resp_wait(devinfo))
>> + if (!brcmf_usb_ioctl_resp_wait(devinfo)) {
>> + usb_unlink_urb(devinfo->ctl_urb);
>
> This is the asynchronous unlink. You have no guarantee it is finished
> after this point.
>
>> ret = -ETIMEDOUT;
>> - else
>> + } else {
>> memcpy(buffer, tmpbuf, buflen);
>> + }
>>
>> finalize:
>> kfree(tmpbuf);
>
> Which means that you are freeing memory that may still be used by DMA
> at this time.
> In addition you have no guarantee that the unlink is indeed finished
> by the time the URB is reused.
> If you wish to take this approach you better forget about this URB
> and allocate a new one and free the buffer from the callback.
Hi Oliver,
Good catch. I think the DMA issue is also present in the current driver: it
frees the buffer without unlinking/killing the URB at all. Can a malicious USB
device force a timeout to occur (i.e. delay the call to the completion
handler)? If so this might be a use-after-free vulnerability.
It seems using usb_kill_urb instead of usb_unlink_urb in the patch prevents any
possible use-after-free. Can someone double check?
Kind regards,
Mathy
>
> Regards
> Oliver
>
^ permalink raw reply
* Re: [PATCH v2 07/19] selftests/firmware: add install target to enable installing test
From: Shuah Khan @ 2014-11-12 1:06 UTC (permalink / raw)
To: Kees Cook
Cc: Greg KH, Andrew Morton, Michal Marek, David S. Miller, Phong Tran,
David Herrmann, Hugh Dickins, pranith kumar, Eric W. Biederman,
Serge E. Hallyn, linux-kbuild, LKML, Linux API,
Network Development
In-Reply-To: <CAGXu5j+yLqH0xj=5N90LRmK9F1xPLVWBy+cyUCzjvKJH7gE7AA@mail.gmail.com>
On 11/11/2014 02:29 PM, Kees Cook wrote:
> Hi,
>
> Sorry, I still really don't like this approach. While it is all in one
> place (thank you for that), I think it isn't a form that is very
> workable for the people maintaining the self tests. How about this,
> instead of per-Makefile customization, why not define an execution
> framework for these tests instead.
If I understand correctly, sounds like you don't like the way
install target is implemented in the individual test Makefiles
and the changes I made to run_tests targets to address the code
duplication concern.
At the moment there is no duplicate code in this patch series
between install and run_tests targets. This is a first step
towards standardizing the framework and a definite improvement
over what we have currently. As I mentioned earlier, my goal
is to make it easier for developers to install and run the
existing tests and evolve the framework as we go.
Assuming my understanding is correct that:
-- install and run_tests targets in individual tests can be
refined and automated with a common Makefile approach you
proposed.
-- the rest of the user-interface kselftest_install and kselftest
are good.
I would like to propose that we get started with the current
implementation and refine it based on the following ideas you
suggested. The refinements you are recommending are confined
to selftests and can be made after the kselftest_install
gets added. Adding kselftest_install makes it easier to make
the refinements as it defines overall UI.
>
> For example, how about every test directory must have a Makefile with
> the needed binary targets. A common makefile could be included that
> defines the "run_tests" target that calls the script "run_tests.sh"
> that is a shell script in the current directory. (For inspiration, see
> how kernel modules can be built out of tree.)
>
> The "run_tests.sh" scripts could all include a common shell script,
> say "../common.sh" that provides any common variables, functions, etc
> (e.g. things like "Start $name test ..." should be in common.sh
> instead of repeated in every script, the installation logic can be in
> once place instead of repeated).
>
> Then along side common.sh could be "run_installed_tests.sh" or
> something, used on the installed target, that would traverse each
> directory, etc. From this, we can have a much more data-driven
> framework, and a common approach to running tests.
>
> As such, we should declare up front how tests should behave on
> failure. And the top-level test runner can do things like count the
> number of tests, failures, etc.
>
> Then, instead of splitting up the patches by test directory, you can
> split them up by logical changes (e.g. defining the common "run_tests"
> target, and then removing the target from all the tests by including
> the common makefile stub that defines it).
>
These are good ideas and I am with on evolving the framework to make
it easier to maintain individual tests. Patches are welcome.
thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978
^ permalink raw reply
* RE: [PATCH net-next 2/2] r8152: adjust rtl_start_rx
From: Hayes Wang @ 2014-11-12 1:45 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, nic_swsd, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org
In-Reply-To: <20141107.113522.837502028522211960.davem@davemloft.net>
David Miller [mailto:davem@davemloft.net]
> Sent: Saturday, November 08, 2014 12:35 AM
[...]
> Does this even work?
>
> If you leave a hole in the ring, the device is going to stop there
> anyways.
>
> So better to replenish the next time you call into this function
> rather than leaving gaps in your receive ring.
Excuse me. Is this still unacceptable?
Should I remove this patch for keeping the original flow?
Best Regards,
Hayes
^ permalink raw reply
* [PATCH net-next v3 0/3] Code adjustment
From: Hayes Wang @ 2014-11-12 2:05 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
In-Reply-To: <1394712342-15778-79-Taiwan-albertk@realtek.com>
v3:
Remove the test_bit for patch #2.
v2:
Correct the spelling error for the comment of patch #3.
v1:
Adjust some codes to make them more reasonable.
Hayes Wang (3):
r8152: remove the duplicate init for the list of rx_done
r8152: clear the flag of SCHEDULE_TASKLET in tasklet
r8152: check RTL8152_UNPLUG and netif_running before autoresume
drivers/net/usb/r8152.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
--
1.9.3
^ permalink raw reply
* [PATCH net-next v3 1/3] r8152: remove the duplicate init for the list of rx_done
From: Hayes Wang @ 2014-11-12 2:05 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
In-Reply-To: <1394712342-15778-91-Taiwan-albertk@realtek.com>
The INIT_LIST_HEAD(&tp->rx_done) would be done in rtl_start_rx(),
so remove the unnecessary one in alloc_all_mem().
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
drivers/net/usb/r8152.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 66b139a..a300467 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1255,7 +1255,6 @@ static int alloc_all_mem(struct r8152 *tp)
spin_lock_init(&tp->rx_lock);
spin_lock_init(&tp->tx_lock);
- INIT_LIST_HEAD(&tp->rx_done);
INIT_LIST_HEAD(&tp->tx_free);
skb_queue_head_init(&tp->tx_queue);
--
1.9.3
^ permalink raw reply related
* [PATCH net-next v3 2/3] r8152: clear the flag of SCHEDULE_TASKLET in tasklet
From: Hayes Wang @ 2014-11-12 2:05 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
In-Reply-To: <1394712342-15778-91-Taiwan-albertk@realtek.com>
Clear the flag of SCHEDULE_TASKLET in bottom_half() to avoid
re-schedule the tasklet again by workqueue.
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
drivers/net/usb/r8152.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index a300467..ad9dd7d 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1797,6 +1797,8 @@ static void bottom_half(unsigned long data)
if (!netif_carrier_ok(tp->netdev))
return;
+ clear_bit(SCHEDULE_TASKLET, &tp->flags);
+
rx_bottom(tp);
tx_bottom(tp);
}
--
1.9.3
^ permalink raw reply related
* [PATCH net-next v3 3/3] r8152: check RTL8152_UNPLUG and netif_running before autoresume
From: Hayes Wang @ 2014-11-12 2:05 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
In-Reply-To: <1394712342-15778-91-Taiwan-albertk@realtek.com>
If the device is unplugged or !netif_running(), the workqueue
doesn't need to wake the device, and could return directly.
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
drivers/net/usb/r8152.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ad9dd7d..0a30fd3 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -2857,15 +2857,18 @@ static void rtl_work_func_t(struct work_struct *work)
{
struct r8152 *tp = container_of(work, struct r8152, schedule.work);
+ /* If the device is unplugged or !netif_running(), the workqueue
+ * doesn't need to wake the device, and could return directly.
+ */
+ if (test_bit(RTL8152_UNPLUG, &tp->flags) || !netif_running(tp->netdev))
+ return;
+
if (usb_autopm_get_interface(tp->intf) < 0)
return;
if (!test_bit(WORK_ENABLE, &tp->flags))
goto out1;
- if (test_bit(RTL8152_UNPLUG, &tp->flags))
- goto out1;
-
if (!mutex_trylock(&tp->control)) {
schedule_delayed_work(&tp->schedule, 0);
goto out1;
--
1.9.3
^ permalink raw reply related
* Re: [PATCH net-next 2/2] r8152: adjust rtl_start_rx
From: David Miller @ 2014-11-12 2:19 UTC (permalink / raw)
To: hayeswang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <0835B3720019904CB8F7AA43166CEEB2ECE337@RTITMBSV03.realtek.com.tw>
From: Hayes Wang <hayeswang@realtek.com>
Date: Wed, 12 Nov 2014 01:45:08 +0000
> David Miller [mailto:davem@davemloft.net]
>> Sent: Saturday, November 08, 2014 12:35 AM
> [...]
>> Does this even work?
>>
>> If you leave a hole in the ring, the device is going to stop there
>> anyways.
>>
>> So better to replenish the next time you call into this function
>> rather than leaving gaps in your receive ring.
>
> Excuse me. Is this still unacceptable?
> Should I remove this patch for keeping the original flow?
I haven't had time to process your original reply, please be patient.
^ permalink raw reply
* [PATCH nf] ipvs: Keep skb->sk when allocating headroom on tunnel xmit
From: Simon Horman @ 2014-11-12 2:22 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: lvs-devel, netdev, netfilter-devel, Wensong Zhang,
Julian Anastasov, Calvin Owens, Simon Horman
In-Reply-To: <1415758920-32578-1-git-send-email-horms@verge.net.au>
From: Calvin Owens <calvinowens@fb.com>
ip_vs_prepare_tunneled_skb() ignores ->sk when allocating a new
skb, either unconditionally setting ->sk to NULL or allowing
the uninitialized ->sk from a newly allocated skb to leak through
to the caller.
This patch properly copies ->sk and increments its reference count.
Signed-off-by: Calvin Owens <calvinowens@fb.com>
Acked-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
net/netfilter/ipvs/ip_vs_xmit.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index 437a366..bd90bf8 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -846,6 +846,8 @@ ip_vs_prepare_tunneled_skb(struct sk_buff *skb, int skb_af,
new_skb = skb_realloc_headroom(skb, max_headroom);
if (!new_skb)
goto error;
+ if (skb->sk)
+ skb_set_owner_w(new_skb, skb->sk);
consume_skb(skb);
skb = new_skb;
}
--
2.1.1
^ permalink raw reply related
* [GIT PULL nf] Second Round of IPVS Fixes for v3.18
From: Simon Horman @ 2014-11-12 2:21 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: lvs-devel, netdev, netfilter-devel, Wensong Zhang,
Julian Anastasov, Simon Horman
Hi Pablo,
please consider this fix for v3.18.
It fixes handling of skb->sk which may cause incorrect handling
of connections from a local process.
This problem was introduced in its current form by 8052ba292559f907e
("ipvs: support ipv4 in ipv6 and ipv6 in ipv4 tunnel forwarding") in
v3.18-rc1.
I believe it also exists in a different form in older kernels.
No fix for that is available at this time.
The following changes since commit 2196937e12b1b4ba139806d132647e1651d655df:
netfilter: ipset: small potential read beyond the end of buffer (2014-11-11 13:46:37 +0100)
are available in the git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs.git tags/ipvs-fixes2-for-v3.18
for you to fetch changes up to 50656d9df63d69ce399c8be62d4473b039dac36a:
ipvs: Keep skb->sk when allocating headroom on tunnel xmit (2014-11-12 11:03:04 +0900)
----------------------------------------------------------------
Calvin Owens (1):
ipvs: Keep skb->sk when allocating headroom on tunnel xmit
net/netfilter/ipvs/ip_vs_xmit.c | 2 ++
1 file changed, 2 insertions(+)
^ permalink raw reply
* Re: [PATCH] ipvs: Keep skb->sk when allocating headroom on tunnel xmit
From: Simon Horman @ 2014-11-12 2:22 UTC (permalink / raw)
To: Julian Anastasov
Cc: Calvin Owens, Wensong Zhang, lvs-devel, linux-kernel, netdev,
agartrell, kernel-team
In-Reply-To: <alpine.LFD.2.11.1411080814300.2081@ja.home.ssi.bg>
On Sat, Nov 08, 2014 at 08:16:58AM +0200, Julian Anastasov wrote:
>
> Hello,
>
> On Fri, 7 Nov 2014, Calvin Owens wrote:
>
> > On 11/05/2014 01:21 AM, Julian Anastasov wrote:
> > >
> > > Hello,
> > >
> > > On Tue, 4 Nov 2014, Calvin Owens wrote:
> > >
> > > > ip_vs_prepare_tunneled_skb() ignores ->sk when allocating a new
> > > > skb, either unconditionally setting ->sk to NULL or allowing
> > > > the uninitialized ->sk from a newly allocated skb to leak through
> > > > to the caller.
> > > >
> > > > This patch properly copies ->sk and increments its reference count.
> > > >
> > > > Signed-off-by: Calvin Owens <calvinowens@fb.com>
> > >
> > > Good catch. Please, extend your patch to
> > > fix also the second place that has such error,
> > > ip_vs_tunnel_xmit_v6. This call is missing from long time,
> > > it was not needed. But commits that allow skb->sk (local
> > > clients) already need it, eg.
> >
> > I'm not sure where exactly you mean: ip_vs_tunnel_xmit_v6() calls
> > ip_vs_prepare_tunneled_skb() to do the allocation, so this patch covers that
> > case.
> >
> > In older versions of the kernel, ip_vs_tunnel_xmit_v6() does it directly,
> > could that be what you're looking at?
>
> Sorry, it seems I was checking old branch.
>
> Simon, please apply.
>
> Acked-by: Julian Anastasov <ja@ssi.bg>
Thanks, done.
> > > - f2428ed5e7bc89c7 ("ipvs: load balance ipv6 connections from a local
> > > process"), 2.6.28
> > > - 4856c84c1358b798 ("ipvs: load balance IPv4 connections from a local
> > > process"), 2.6.28
> > >
> > > > ---
> > > > net/netfilter/ipvs/ip_vs_xmit.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/net/netfilter/ipvs/ip_vs_xmit.c
> > > > b/net/netfilter/ipvs/ip_vs_xmit.c
> > > > index 437a366..bd90bf8 100644
> > > > --- a/net/netfilter/ipvs/ip_vs_xmit.c
> > > > +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> > > > @@ -846,6 +846,8 @@ ip_vs_prepare_tunneled_skb(struct sk_buff *skb, int
> > > > skb_af,
> > > > new_skb = skb_realloc_headroom(skb, max_headroom);
> > > > if (!new_skb)
> > > > goto error;
> > > > + if (skb->sk)
> > > > + skb_set_owner_w(new_skb, skb->sk);
> > > > consume_skb(skb);
> > > > skb = new_skb;
> > > > }
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
>
^ permalink raw reply
* Re: [GIT PULL nf] IPVS Fixes for v3.18
From: Simon Horman @ 2014-11-12 2:24 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: lvs-devel, netdev, netfilter-devel, Wensong Zhang,
Julian Anastasov
In-Reply-To: <20141104175717.GA25184@salvia>
On Tue, Nov 04, 2014 at 06:57:17PM +0100, Pablo Neira Ayuso wrote:
> Hi Simon,
>
> On Tue, Oct 28, 2014 at 10:05:33AM +0900, Simon Horman wrote:
> > Hi Pablo,
> >
> > please consider this fix for v3.18.
> >
> > It fixes a null-pointer dereference that may occur when logging
> > errors.
> >
> > This problem was introduced by 4a4739d56b0 ("ipvs: Pull out
> > crosses_local_route_boundary logic") in v3.17-rc5. As such I would
> > also like it considered for 3.17-stable.
>
> Regarding your request to pass this to 3.17-stable. According to git
> am and my scripts:
>
> 3d53666 ipvs: Avoid null-pointer deref in debug code
>
> doesn't apply cleanly 3.17.x. Please re-check and send me a backport if
> you want to see this in 3.17.x. Let me know, sorry.
Sorry for creating some confusion here and sorry for not
cleaning it up earlier.
On further inspection I believe that 4a4739d56b0 was included
in v3.18-rc1 rather than v3.17. So this fix is not relevant to
v3.17 after all.
^ permalink raw reply
* Re: [PATCH net-next 1/1] ipvlan: Initial check-in of the IPVLAN driver.
From: Eric Dumazet @ 2014-11-12 2:29 UTC (permalink / raw)
To: Cong Wang
Cc: Hannes Frederic Sowa, Mahesh Bandewar, netdev, Eric Dumazet,
Maciej Zenczykowski, Laurent Chavey, Tim Hockin, David Miller,
Brandon Philips, Pavel Emelianov
In-Reply-To: <CAHA+R7MgjOKuVY_egNz2SOWZG6X2rkFGfcaP0PiMKc_YVKAULA@mail.gmail.com>
On Tue, 2014-11-11 at 16:39 -0800, Cong Wang wrote:
> Sounds like over-kill to have a new device just for not worrying about mac.
> Or you mean our neigh table doesn't scale?
Some environments simply do not allow having multiple MAC, it is not a
linux problem with neigh table.
Linux hosts can be attached to switches with a very strict security
policy : One (or few) mac address per port.
http://en.wikipedia.org/wiki/CAM_Table
^ permalink raw reply
* RE: [PATCH net-next 2/2] r8152: adjust rtl_start_rx
From: Hayes Wang @ 2014-11-12 2:30 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, nic_swsd, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org
In-Reply-To: <20141111.211939.1875418281655170486.davem@davemloft.net>
David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, November 12, 2014 10:20 AM
[...]
> I haven't had time to process your original reply, please be patient.
I am sorry for bothering you, and thanks for your reply.
Best Regards,
Hayes
^ permalink raw reply
* Re: [PATCH] selftests/net: psock_fanout seg faults in sock_fanout_read_ring()
From: David Miller @ 2014-11-12 2:44 UTC (permalink / raw)
To: shuahkh; +Cc: netdev, linux-api, linux-kernel
In-Reply-To: <1415725453-6549-1-git-send-email-shuahkh@osg.samsung.com>
From: Shuah Khan <shuahkh@osg.samsung.com>
Date: Tue, 11 Nov 2014 10:04:13 -0700
> The while loop in sock_fanout_read_ring() checks mmap region
> bounds after access, causing it to segfault. Fix it to check
> count before accessing header->tp_status. This problem can be
> reproduced consistently when the test in run as follows:
>
> make -C tools/testing/selftests TARGETS=net run_tests
> or
> make run_tests from tools/testing/selftests
> or
> make run_test from tools/testing/selftests/net
>
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
Definitely looks correct to me, applied, thanks.
^ 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