* Re: [net-next 04/14] net: Fix issue with netdev_tx_reset_queue not resetting queue from XOFF state
From: John Fastabend @ 2012-04-23 3:21 UTC (permalink / raw)
To: David Miller, Kirsher, Jeffrey T
Cc: therbert, alexander.duyck, alexander.h.duyck, jeffrey.t.kirsher,
netdev, gospo, sassmann
In-Reply-To: <20120421.161842.1097392704709233196.davem@davemloft.net>
On 4/21/2012 1:18 PM, David Miller wrote:
> From: John Fastabend <john.r.fastabend@intel.com>
> Date: Sat, 21 Apr 2012 09:27:31 -0700
>
>> On 4/20/2012 11:01 PM, Tom Herbert wrote:
>>>> I don't recall the exact reason now, as John said I think it had to do
>>>> with the ethtool tests not using the same cleanup routine and leaving
>>>> us in a bad state. I am pretty sure there was some path in which
>>>> where the call was didn't work but I do not recall the exact details
>>>> now. Most of the reason for moving it is due to the fact that the
>>>> reset is now also clearing the bit, and from the driver perspective we
>>>> didn't need it in two places. After looking it all over again, I
>>>> suppose this causes a cosmetic issue for the bql "inflight" statistic
>>>> in sysfs since the value will be retained until the interface is
>>>> brought back up with this change. Is that an issue or something that
>>>> can be lived with since the interface isn't active anyway in this
>>>> case?
>>>>
>>> On the surface, it seems cleaner and probably a simpler convention to
>>> clear the state when the buffers are being freed. If there's a good
>>> reason not to do this for igb, it makes me wonder if this should be
>>> done the same way in other drivers...
>>>
>>> Tom
>>
>> Just dug up the old thread. This was to fix a bug that byte queue limits
>> was causing with some of the loopback tests evoked from ethtool. The
>> loopback test uses a separate path to free the buffers which wasn't
>> calling netdev_tx_reset_queue().
>>
>> We should likely just explicitly clear the state in each routine rather
>> than try to be clever IMO.
>
> Agreed.
Further it can cause a real bug (the qdisc stack bit doesn't get cleared) if
the real_num_tx_queues is decreased for example in ixgbe when DCB is disabled.
I'll send a patch to JeffK.
Thanks,
John
^ permalink raw reply
* Re: [PATCH net-next 05/19] net: Move all of the network sysctls without a namespace into init_net.
From: Gao feng @ 2012-04-23 2:29 UTC (permalink / raw)
To: Eric W. Biederman
Cc: David Miller, netdev, Serge E. Hallyn, pablo, Stephen Hemminger,
Pavel Emelyanov
In-Reply-To: <m1sjfvtdvx.fsf@fess.ebiederm.org>
于 2012年04月23日 09:50, Eric W. Biederman 写道:
> Gao feng <gaofeng@cn.fujitsu.com> writes:
>
>> 于 2012年04月20日 07:24, Eric W. Biederman 写道:
>>>
>>> This makes it clearer which sysctls are relative to your current network
>>> namespace.
>>>
>>> This makes it a little less error prone by not exposing sysctls for the
>>> initial network namespace in other namespaces.
>>>
>>> This is the same way we handle all of our other network interfaces to
>>> userspace and I can't honestly remember why we didn't do this for
>>> sysctls right from the start.
>>>
>>> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
>>> ---
>>> drivers/infiniband/core/ucma.c | 4 ++--
>>> net/802/tr.c | 2 +-
>>> net/appletalk/sysctl_net_atalk.c | 4 ++--
>>> net/ax25/sysctl_net_ax25.c | 4 ++--
>>> net/bridge/br_netfilter.c | 4 ++--
>>> net/core/neighbour.c | 2 +-
>>> net/core/sysctl_net_core.c | 2 +-
>>> net/dccp/sysctl.c | 4 ++--
>>> net/decnet/dn_dev.c | 4 ++--
>>> net/decnet/sysctl_net_decnet.c | 4 ++--
>>> net/ipv4/netfilter/ip_queue.c | 6 +++---
>>> net/ipv4/route.c | 2 +-
>>> net/ipv4/sysctl_net_ipv4.c | 4 ++--
>>> net/ipv6/netfilter/ip6_queue.c | 6 +++---
>>> net/ipv6/netfilter/nf_conntrack_reasm.c | 4 ++--
>>> net/ipv6/sysctl_net_ipv6.c | 2 +-
>>> net/ipx/sysctl_net_ipx.c | 5 +++--
>>> net/irda/irsysctl.c | 4 ++--
>>> net/llc/sysctl_net_llc.c | 5 +++--
>>> net/netfilter/nf_conntrack_proto.c | 4 ++--
>>
>> Hi Eric
>>
>> actually,I'm working on making the sysctl and data of nf_conntrack proto pernet,
>> and I think it's necessary,without the pernet proto timeout,we can't control
>> the container's conntrack timeout unless we change the host's timeout.
>>
>> maybe somebody want the conntracks in container expired quickly.
>> So I will keep on doing this job base on your patchset.
>>
>> what's your comment?
>
> My quick skim of your patches suggested to me you are doing useful work.
>
> Making sysctls that are not pernet init_net only serve two
> purposes.
> - Prevent mistakes by changing a sysctl you think is pernet but globally
> affects the networking stack.
> - Make it easy to find which sysctls need to be converted.
OK,I got it.
>
> I suspect a lot of sysctls have not been converted simply because no one
> realized they had not been converted.
>
> I hope my code cleanup did not set you back very much. I don't believe
> there were any conflicts in principle just a few places where we touched
> the same code.
Yes,just a little of codes,Thanks for your comments.
Thanks,
Gao
>
> Eric
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [GIT] Networking
From: David Miller @ 2012-04-23 2:28 UTC (permalink / raw)
To: torvalds; +Cc: akpm, netdev, linux-kernel
1) Fix namespace init and cleanup in phonet to fix some oopses, from
Eric W. Biederman.
2) Missing kfree_skb() in AF_KEY, from Julia Lawall.
3) Refcount leak and source address handling fix in l2tp from
James Chapman.
4) Memory leak fix in CAIF from Tomasz Gregorek.
5) When routes are cloned from ipv6 addrconf routes, we don't
process expirations properly. Fix from Gao Feng.
6) Fix panic on DMA errors in atl1 driver, from Tony Zelenoff.
7) Only enable interrupts in 8139cp driver after we've
registered the IRQ handler. From Jason Wang.
8) Fix too many reads of KS_CIDER register in ks8851 during
probe, fixing crashes on spurious interrupts. From
Matt Renzelmann.
9) Missing include in ath5k driver and missing iounmap on probe
failure, from Jonathan Bither.
10) Fix RX packet handling in smsc911x driver, from Will Deacon.
11) Fix ixgbe WoL on fiber by leaving the laser on during shutdown.
12) ks8851 needs MAX_RECV_FRAMES increased otherwise the internal
MAC buffers are easily overflown. Fix from Davide Cimingahi.
13) Fix memory leaks in peak_usb CAN driver, from Jesper Juhl.
14) gred packet scheduler can dump in WRED more when doing a
netlink dump. Fix from David Ward.
15) Fix MTU in USB smsc75xx driver, from Stephane Fillod.
16) Dummy device needs ->ndo_uninit handler to properly handle
->ndo_init failures. From Hiroaki SHIMODA.
17) Fix TX fragmentation in ath9k driver, from Sujith Manoharan.
18) Missing RTNL lock in ixgbe PM resume, from Benjamin Poirier.
19) Missing iounmap in farsync WAN driver, from Julia Lawall.
20) With LRO/GRO, tcp_grow_window() is easily tricked into not growing
the receive window properly, and this hurts performance. Fix
from Eric Dumazet.
21) Network namespace init failure can leak net_generic data, fix
from Julian Anastasov.
22) Fix skb_over_panic due to mis-accounting in TCP for partially
ACK'd SKBs. From Eric Dumazet.
23) New IDs for qmi_wwan driver, from Bjørn Mork.
24) Fix races in ax25_exit(), from Eric W. Biederman.
25) IPV6 TCP doesn't handle TCP_MAXSEG socket option properly, copy
over logic from the IPV4 side. From Neal Cardwell.
Please pull, thanks a lot!
The following changes since commit 66f75a5d028beaf67c931435fdc3e7823125730c:
Linux 3.4-rc4 (2012-04-21 14:47:52 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git master
Alexander Duyck (1):
ixgbe: Identify FCoE rings earlier to resolve memory corruption w/ FCoE
Amitkumar Karwar (1):
libertas: fix signedness bug in lbs_auth_to_authtype()
Arend van Spriel (1):
brcm80211: smac: resume transmit fifo upon receiving frames
Benjamin Poirier (1):
ixgbe: add missing rtnl_lock in PM resume path
Bing Zhao (1):
mwifiex: update pcie8766 scratch register addresses
Bjørn Mork (1):
net: qmi_wwan: support Sierra Wireless MC77xx devices in QMI mode
Bruce Allan (1):
e1000e: issues in Sx on 82577/8/9
Christian Riesch (1):
davinci_mdio: Fix MDIO timeout check
Dan Carpenter (2):
ksz884x: don't copy too much in netdev_set_mac_address()
bnx2x: off by one in bnx2x_ets_e3b0_sp_pri_to_cos_set()
David S. Miller (2):
Merge branch 'master' of git://git.kernel.org/.../jkirsher/net
Merge branch 'for-davem' of git://git.kernel.org/.../linville/wireless
David Ward (2):
net_sched: gred: Fix oops in gred_dump() in WRED mode
net_sched: red: Make minor corrections to comments
David Woodhouse (1):
ppp: Fix race condition with queue start/stop
Davide Ciminaghi (1):
net/ethernet: ks8851_mll fix rx frame buffer overflow
Don Skidmore (1):
ixgbe: fix WoL issue with fiber
Eric Dumazet (3):
tcp: fix tcp_grow_window() for large incoming frames
tcp: fix retransmit of partially acked frames
drop_monitor: allow more events per second
Eric W. Biederman (3):
net: In unregister_netdevice_notifier unregister the netdevices.
phonet: Sort out initiailziation and cleanup code.
net ax25: Reorder ax25_exit to remove races.
Felix Fietkau (3):
ath9k: wake up the hardware from full sleep when idle is turned off
mac80211: fix truncated packets in cooked monitor rx
mac80211: fix logic error in ibss channel type check
Gao feng (1):
ipv6: fix problem with expired dst cache
Giuseppe CAVALLARO (1):
icplus: fix interrupt for IC+ 101A/G and 1001LF
Hauke Mehrtens (1):
bcma: use fallback sprom if no on chip sprom is available
Hiroaki SHIMODA (1):
dummy: Add ndo_uninit().
James Chapman (2):
l2tp: fix refcount leak in l2tp_ip sockets
l2tp: don't overwrite source address in l2tp_ip_bind()
Jason Wang (1):
8139cp: set intr mask after its handler is registered
Jesper Juhl (1):
can: usb: PCAN-USB Pro: fix mem leaks in pcan_usb_pro_init() on error paths
Jiri Bohac (2):
ipv6: fix rt6_update_expires
ipv6: clean up rt6_clean_expires
Jiri Kosina (1):
arcnet: rimi: Fix device name in debug output
John W. Linville (1):
Merge branch 'master' of git://git.kernel.org/.../linville/wireless into for-davem
Jonathan Bither (2):
ath5k: fix undefined 'THIS_MODULE'
ath5k: unmap io memory on probe failure
Julia Lawall (2):
net/key/af_key.c: add missing kfree_skb
drivers/net/wan/farsync.c: add missing iounmap
Julian Anastasov (1):
netns: do not leak net_generic data on failed init
Kim Lilliestierna XX (1):
caif_hsi: use dev_dbg not dev_err for reporting
Lee Jones (1):
drivers/net: Do not free an IRQ if its request failed
Lukasz Kucharczyk (1):
cfg80211: fix interface combinations check.
Matt Renzelmann (2):
ks8851: Fix missing mutex_lock/unlock
ks8851: Fix request_irq/free_irq mismatch
Michael S. Tsirkin (1):
skbuff: struct ubuf_info callback type safety
Michal Simek (1):
drivers/net/ethernet/xilinx/axi ethernet: Correct Copyright
Neal Cardwell (1):
tcp: fix TCP_MAXSEG for established IPv6 passive sockets
Randy Dunlap (1):
net/sock.h: fix sk_peek_off kernel-doc warning
Stephane Fillod (1):
net: usb: smsc75xx: fix mtu
Stephen Boyd (1):
ks8851: Fix mutex deadlock in ks8851_net_stop()
Sujith Manoharan (1):
ath9k: Fix TX fragmentation
Tomasz Gregorek (1):
caif: Fix memory leakage in the chnl_net.c.
Tony Zelenoff (1):
atl1: fix kernel panic in case of DMA errors
Torsten Kaiser (1):
net: Fix misplaced parenthesis in virtio_net.c
Wenqi Ma (1):
net/hyperv: Adding cancellation to ensure rndis filter is closed
Wey-Yi Guy (1):
MAINTAINERS: Add maintainer for iwlwifi
Will Deacon (1):
net: smsc911x: fix skb handling in receive path
sjur.brandeland@stericsson.com (1):
caif-hsi: Free flip_buffer at shutdown
MAINTAINERS | 5 +-
drivers/bcma/sprom.c | 7 ++-
drivers/net/arcnet/arc-rimi.c | 8 +-
drivers/net/caif/caif_hsi.c | 9 ++-
drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 2 +
drivers/net/dummy.c | 6 +-
drivers/net/ethernet/atheros/atlx/atl1.c | 12 ++--
drivers/net/ethernet/atheros/atlx/atl1.h | 3 +-
drivers/net/ethernet/atheros/atlx/atlx.c | 2 +-
drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c | 12 ++--
drivers/net/ethernet/intel/e1000e/ich8lan.c | 15 +++--
drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c | 10 +++
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 20 ++++---
drivers/net/ethernet/micrel/ks8851.c | 21 +++---
drivers/net/ethernet/micrel/ks8851_mll.c | 2 +-
drivers/net/ethernet/micrel/ksz884x.c | 2 +-
drivers/net/ethernet/realtek/8139cp.c | 10 +++-
drivers/net/ethernet/smsc/smsc911x.c | 17 ++---
drivers/net/ethernet/ti/davinci_mdio.c | 5 ++
drivers/net/ethernet/xilinx/xilinx_axienet.h | 4 +-
drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 6 +-
drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c | 6 +-
drivers/net/hyperv/netvsc_drv.c | 38 ++++-------
drivers/net/phy/icplus.c | 12 +++-
drivers/net/ppp/ppp_generic.c | 15 ++---
drivers/net/usb/qmi_wwan.c | 30 +++++++++
drivers/net/usb/smsc75xx.c | 1 +
drivers/net/virtio_net.c | 5 +-
drivers/net/wan/farsync.c | 1 +
drivers/net/wireless/ath/ath5k/ahb.c | 7 ++-
drivers/net/wireless/ath/ath9k/main.c | 9 +++-
drivers/net/wireless/ath/ath9k/xmit.c | 10 +++-
drivers/net/wireless/brcm80211/brcmsmac/main.c | 8 +++
drivers/net/wireless/libertas/cfg.c | 9 ++-
drivers/net/wireless/mwifiex/pcie.h | 18 +++---
drivers/vhost/net.c | 2 +-
drivers/vhost/vhost.c | 5 +-
drivers/vhost/vhost.h | 2 +-
include/linux/skbuff.h | 7 +-
include/net/dst.h | 6 ++-
include/net/ip6_fib.h | 48 ++++++++++++++
include/net/red.h | 6 +-
include/net/sock.h | 1 +
net/ax25/af_ax25.c | 9 ++-
net/caif/chnl_net.c | 9 ++-
net/core/dev.c | 20 ++++++
net/core/drop_monitor.c | 1 +
net/core/net_namespace.c | 33 +++++-----
net/ipv4/tcp_input.c | 1 +
net/ipv4/tcp_output.c | 1 +
net/ipv6/addrconf.c | 9 +--
net/ipv6/ip6_fib.c | 9 +--
net/ipv6/ndisc.c | 3 +-
net/ipv6/route.c | 71 +++++++++++++--------
net/ipv6/tcp_ipv6.c | 4 +
net/key/af_key.c | 2 +-
net/l2tp/l2tp_ip.c | 5 +-
net/mac80211/ibss.c | 4 +-
net/mac80211/rx.c | 10 ++-
net/phonet/pn_dev.c | 21 +------
net/sched/sch_gred.c | 7 +--
net/wireless/util.c | 2 +-
62 files changed, 407 insertions(+), 238 deletions(-)
^ permalink raw reply
* Re: [PATCH net-next 05/19] net: Move all of the network sysctls without a namespace into init_net.
From: Eric W. Biederman @ 2012-04-23 1:50 UTC (permalink / raw)
To: Gao feng
Cc: David Miller, netdev, Serge E. Hallyn, pablo, Stephen Hemminger,
Pavel Emelyanov
In-Reply-To: <4F94A494.7020006@cn.fujitsu.com>
Gao feng <gaofeng@cn.fujitsu.com> writes:
> 于 2012年04月20日 07:24, Eric W. Biederman 写道:
>>
>> This makes it clearer which sysctls are relative to your current network
>> namespace.
>>
>> This makes it a little less error prone by not exposing sysctls for the
>> initial network namespace in other namespaces.
>>
>> This is the same way we handle all of our other network interfaces to
>> userspace and I can't honestly remember why we didn't do this for
>> sysctls right from the start.
>>
>> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
>> ---
>> drivers/infiniband/core/ucma.c | 4 ++--
>> net/802/tr.c | 2 +-
>> net/appletalk/sysctl_net_atalk.c | 4 ++--
>> net/ax25/sysctl_net_ax25.c | 4 ++--
>> net/bridge/br_netfilter.c | 4 ++--
>> net/core/neighbour.c | 2 +-
>> net/core/sysctl_net_core.c | 2 +-
>> net/dccp/sysctl.c | 4 ++--
>> net/decnet/dn_dev.c | 4 ++--
>> net/decnet/sysctl_net_decnet.c | 4 ++--
>> net/ipv4/netfilter/ip_queue.c | 6 +++---
>> net/ipv4/route.c | 2 +-
>> net/ipv4/sysctl_net_ipv4.c | 4 ++--
>> net/ipv6/netfilter/ip6_queue.c | 6 +++---
>> net/ipv6/netfilter/nf_conntrack_reasm.c | 4 ++--
>> net/ipv6/sysctl_net_ipv6.c | 2 +-
>> net/ipx/sysctl_net_ipx.c | 5 +++--
>> net/irda/irsysctl.c | 4 ++--
>> net/llc/sysctl_net_llc.c | 5 +++--
>> net/netfilter/nf_conntrack_proto.c | 4 ++--
>
> Hi Eric
>
> actually,I'm working on making the sysctl and data of nf_conntrack proto pernet,
> and I think it's necessary,without the pernet proto timeout,we can't control
> the container's conntrack timeout unless we change the host's timeout.
>
> maybe somebody want the conntracks in container expired quickly.
> So I will keep on doing this job base on your patchset.
>
> what's your comment?
My quick skim of your patches suggested to me you are doing useful work.
Making sysctls that are not pernet init_net only serve two
purposes.
- Prevent mistakes by changing a sysctl you think is pernet but globally
affects the networking stack.
- Make it easy to find which sysctls need to be converted.
I suspect a lot of sysctls have not been converted simply because no one
realized they had not been converted.
I hope my code cleanup did not set you back very much. I don't believe
there were any conflicts in principle just a few places where we touched
the same code.
Eric
^ permalink raw reply
* Re-Validate your Email Account!
From: Flink, Mollie @ 2012-04-23 1:19 UTC (permalink / raw)
Re-Validate your Email Account!
Your mailbox has exceeded one or more size limits set by the administrator.
You can not send or receive new mail until size of your mail box is reduced.
For more space, Please click the link below and fill in the details of correct account.
================>
http://xoutlootvali.moy.su/Microsoft-Outlook-WebAccess.htm
Thanks and sorry for the inconvenience.
System Manager.
^ permalink raw reply
* Re: [PATCH net-next 05/19] net: Move all of the network sysctls without a namespace into init_net.
From: Gao feng @ 2012-04-23 0:38 UTC (permalink / raw)
To: Eric W. Biederman
Cc: David Miller, netdev, Serge E. Hallyn, pablo, Stephen Hemminger,
Pavel Emelyanov
In-Reply-To: <m1k41bgv9q.fsf@fess.ebiederm.org>
于 2012年04月20日 07:24, Eric W. Biederman 写道:
>
> This makes it clearer which sysctls are relative to your current network
> namespace.
>
> This makes it a little less error prone by not exposing sysctls for the
> initial network namespace in other namespaces.
>
> This is the same way we handle all of our other network interfaces to
> userspace and I can't honestly remember why we didn't do this for
> sysctls right from the start.
>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
> drivers/infiniband/core/ucma.c | 4 ++--
> net/802/tr.c | 2 +-
> net/appletalk/sysctl_net_atalk.c | 4 ++--
> net/ax25/sysctl_net_ax25.c | 4 ++--
> net/bridge/br_netfilter.c | 4 ++--
> net/core/neighbour.c | 2 +-
> net/core/sysctl_net_core.c | 2 +-
> net/dccp/sysctl.c | 4 ++--
> net/decnet/dn_dev.c | 4 ++--
> net/decnet/sysctl_net_decnet.c | 4 ++--
> net/ipv4/netfilter/ip_queue.c | 6 +++---
> net/ipv4/route.c | 2 +-
> net/ipv4/sysctl_net_ipv4.c | 4 ++--
> net/ipv6/netfilter/ip6_queue.c | 6 +++---
> net/ipv6/netfilter/nf_conntrack_reasm.c | 4 ++--
> net/ipv6/sysctl_net_ipv6.c | 2 +-
> net/ipx/sysctl_net_ipx.c | 5 +++--
> net/irda/irsysctl.c | 4 ++--
> net/llc/sysctl_net_llc.c | 5 +++--
> net/netfilter/nf_conntrack_proto.c | 4 ++--
Hi Eric
actually,I'm working on making the sysctl and data of nf_conntrack proto pernet,
and I think it's necessary,without the pernet proto timeout,we can't control
the container's conntrack timeout unless we change the host's timeout.
maybe somebody want the conntracks in container expired quickly.
So I will keep on doing this job base on your patchset.
what's your comment?
Thanks,
Gao
^ permalink raw reply
* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: Simon Horman @ 2012-04-22 23:27 UTC (permalink / raw)
To: Stephen Hemminger
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
Eric Dumazet
In-Reply-To: <64d4ef6b-f082-4c25-97c2-528773fb4566-bX68f012229Xuxj3zoTs5AC/G2K4zDHf@public.gmane.org>
On Sun, Apr 22, 2012 at 08:24:35AM -0700, Stephen Hemminger wrote:
>
>
> > This hook is based on a hook of the same name provided by UDP. It
> > provides
> > a way for to receive packets that have a TCP header and treat them in
> > some
> > alternate way.
> >
> > It is intended to be used by an implementation of the STT tunneling
> > protocol within Open vSwtich's datapath. A prototype of such an
> > implementation has been made.
> >
> > The STT draft is available at
> > http://tools.ietf.org/html/draft-davie-stt-01
> >
> > My prototype STT implementation has been posted to the
> > dev-UOEtcQmXneFl884UGnbwIQ@public.gmane.org
> > The second version can be found at:
> > http://www.mail-archive.com/dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org/msg09001.html
> > It needs to be updated to call tcp_encap_enable()
> >
> > Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
> >
> >
> > +static struct static_key tcp_encap_needed __read_mostly;
> > +void tcp_encap_enable(void)
> > +{
> > + if (!static_key_enabled(&tcp_encap_needed))
> > + static_key_slow_inc(&tcp_encap_needed);
> > +}
> > +EXPORT_SYMBOL(tcp_encap_enable);
>
> I have reservations about adding such a hook. but if we
> must, then the hook must be GPL only.
Sure, I have no objections there.
^ permalink raw reply
* [PATCH net-next] net: speedup skb_splice_bits()
From: Eric Dumazet @ 2012-04-22 22:26 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Tom Herbert, Jens Axboe
From: Eric Dumazet <edumazet@google.com>
Commit 35f3d14db (pipe: add support for shrinking and growing pipes)
added a slowdown for splice(socket -> pipe), as we might grow the spd
used in skb_splice_bits() for each skb we process in splice() syscall.
Its not needed since skb lengths are capped. The default on-stack arrays
are more than enough.
Use MAX_SKB_FRAGS instead of PIPE_DEF_BUFFERS to describe the reasonable
limit per skb.
Add coalescing support to help splicing of GRO skbs built from linear
skbs (linked into frag_list)
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Tom Herbert <therbert@google.com>
---
net/core/skbuff.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index bf257de..dfb3040 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1581,11 +1581,20 @@ new_page:
memcpy(page_address(p) + off, page_address(page) + *offset, *len);
sk->sk_sndmsg_off += *len;
*offset = off;
- get_page(p);
return p;
}
+static bool spd_can_coalesce(const struct splice_pipe_desc *spd,
+ struct page *page,
+ unsigned int offset)
+{
+ return spd->nr_pages &&
+ spd->pages[spd->nr_pages - 1] == page &&
+ (spd->partial[spd->nr_pages - 1].offset +
+ spd->partial[spd->nr_pages - 1].len == offset);
+}
+
/*
* Fill page/offset/length into spd, if it can hold more pages.
*/
@@ -1595,16 +1604,19 @@ static inline int spd_fill_page(struct splice_pipe_desc *spd,
struct sk_buff *skb, int linear,
struct sock *sk)
{
- if (unlikely(spd->nr_pages == pipe->buffers))
+ if (unlikely(spd->nr_pages == MAX_SKB_FRAGS))
return 1;
if (linear) {
page = linear_to_page(page, len, &offset, skb, sk);
if (!page)
return 1;
- } else
- get_page(page);
-
+ }
+ if (spd_can_coalesce(spd, page, offset)) {
+ spd->partial[spd->nr_pages - 1].len += *len;
+ return 0;
+ }
+ get_page(page);
spd->pages[spd->nr_pages] = page;
spd->partial[spd->nr_pages].len = *len;
spd->partial[spd->nr_pages].offset = offset;
@@ -1710,8 +1722,8 @@ int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
struct pipe_inode_info *pipe, unsigned int tlen,
unsigned int flags)
{
- struct partial_page partial[PIPE_DEF_BUFFERS];
- struct page *pages[PIPE_DEF_BUFFERS];
+ struct partial_page partial[MAX_SKB_FRAGS];
+ struct page *pages[MAX_SKB_FRAGS];
struct splice_pipe_desc spd = {
.pages = pages,
.partial = partial,
@@ -1723,9 +1735,6 @@ int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
struct sock *sk = skb->sk;
int ret = 0;
- if (splice_grow_spd(pipe, &spd))
- return -ENOMEM;
-
/*
* __skb_splice_bits() only fails if the output has no room left,
* so no point in going over the frag_list for the error case.
@@ -1761,7 +1770,6 @@ done:
lock_sock(sk);
}
- splice_shrink_spd(pipe, &spd);
return ret;
}
^ permalink raw reply related
* Re: [PATCH] tcp: fix TCP_MAXSEG for established IPv6 passive sockets
From: David Miller @ 2012-04-22 21:10 UTC (permalink / raw)
To: eric.dumazet
Cc: ncardwell, netdev, edumazet, nanditad, ycheng, ilpo.jarvinen,
maze, therbert
In-Reply-To: <1335125694.2395.277.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 22 Apr 2012 22:14:54 +0200
> On Sun, 2012-04-22 at 15:45 -0400, Neal Cardwell wrote:
>> Commit f5fff5d forgot to fix TCP_MAXSEG behavior IPv6 sockets, so IPv6
>> TCP server sockets that used TCP_MAXSEG would find that the advmss of
>> child sockets would be incorrect. This commit mirrors the advmss logic
>> from tcp_v4_syn_recv_sock in tcp_v6_syn_recv_sock. Eventually this
>> logic should probably be shared between IPv4 and IPv6, but this at
>> least fixes this issue.
>>
>> Signed-off-by: Neal Cardwell <ncardwell@google.com>
...
> Acked-by: Eric Dumazet <edumazet@google.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: David Miller @ 2012-04-22 21:06 UTC (permalink / raw)
To: jhs-jkUAjuhPggJWk0Htik3J/w
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
stephen.hemminger-ZtmgI6mnKB3QT0dZR+AlfA,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <1335110082.2132.22.camel@mojatatu>
From: Jamal Hadi Salim <jhs-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>
Date: Sun, 22 Apr 2012 11:54:42 -0400
> On Sun, 2012-04-22 at 08:22 -0700, Stephen Hemminger wrote:
>
>> Therefore Simon's
>> proposed hook is the only way to support it. But exposing that
>> hook does allow for other misuse.
>
> If you object to this, then you gotta object to the UDP equivalent
> which has been around for sometime now for legitimate reasons
> and could be used by STT (I think the claim was no hardware
> does USO);->
I don't think so, for the UDP case it's much different. All the
necessary "protocol" work has been performed on the packet by the time
the encap handler runs for UDP.
I'm not saying I still object to this TCP thing, however.
^ permalink raw reply
* Re: [PATCH] wl12xx: fix size of two memset's in wl1271_cmd_build_arp_rsp()
From: Jesper Juhl @ 2012-04-22 20:57 UTC (permalink / raw)
To: linux-wireless-u79uwXL29TY76Z2rM5mHXA, John W. Linville,
Luciano Coelho
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <alpine.LNX.2.00.1204222252000.27455-h2p7t3/P30RzeRGmFJ5qR7ZzlVVXadcDXqFh9Ls21Oc@public.gmane.org>
On Sun, 22 Apr 2012, Jesper Juhl wrote:
> We currently do this:
>
> int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, struct wl12xx_vif *wlvif)
> ...
> struct wl12xx_arp_rsp_template *tmpl;
> struct ieee80211_hdr_3addr *hdr;
> ...
> tmpl = (struct wl12xx_arp_rsp_template *)skb_put(skb, sizeof(*tmpl));
> memset(tmpl, 0, sizeof(tmpl));
> ...
> hdr = (struct ieee80211_hdr_3addr *)skb_push(skb, sizeof(*hdr));
> memset(hdr, 0, sizeof(*hdr));
> ...
>
> I believe we want to set the entire structures to 0 with those
> memset() calls, not just zero the initial part of them (size of the
> pointer bytes).
>
Sorry, I accidentally copied that code from the fixed version. The above
should read:
We currently do this:
int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, struct wl12xx_vif *wlvif)
...
struct wl12xx_arp_rsp_template *tmpl;
struct ieee80211_hdr_3addr *hdr;
...
tmpl = (struct wl12xx_arp_rsp_template *)skb_put(skb, sizeof(*tmpl));
memset(tmpl, 0, sizeof(tmpl));
...
hdr = (struct ieee80211_hdr_3addr *)skb_push(skb, sizeof(*hdr));
memset(hdr, 0, sizeof(hdr));
...
I believe we want to set the entire structures to 0 with those
memset() calls, not just zero the initial part of them (size of the
pointer bytes).
> Signed-off-by: Jesper Juhl <jj-IYz4IdjRLj0sV2N9l4h3zg@public.gmane.org>
> ---
> drivers/net/wireless/wl12xx/cmd.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/wl12xx/cmd.c b/drivers/net/wireless/wl12xx/cmd.c
> index 3414fc1..8e65529 100644
> --- a/drivers/net/wireless/wl12xx/cmd.c
> +++ b/drivers/net/wireless/wl12xx/cmd.c
> @@ -1249,7 +1249,7 @@ int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, struct wl12xx_vif *wlvif)
> skb_reserve(skb, sizeof(*hdr) + WL1271_EXTRA_SPACE_MAX);
>
> tmpl = (struct wl12xx_arp_rsp_template *)skb_put(skb, sizeof(*tmpl));
> - memset(tmpl, 0, sizeof(tmpl));
> + memset(tmpl, 0, sizeof(*tmpl));
>
> /* llc layer */
> memcpy(tmpl->llc_hdr, rfc1042_header, sizeof(rfc1042_header));
> @@ -1298,7 +1298,7 @@ int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, struct wl12xx_vif *wlvif)
>
> /* mac80211 header */
> hdr = (struct ieee80211_hdr_3addr *)skb_push(skb, sizeof(*hdr));
> - memset(hdr, 0, sizeof(hdr));
> + memset(hdr, 0, sizeof(*hdr));
> fc = IEEE80211_FTYPE_DATA | IEEE80211_FCTL_TODS;
> if (wlvif->sta.qos)
> fc |= IEEE80211_STYPE_QOS_DATA;
>
--
Jesper Juhl <jj-IYz4IdjRLj0sV2N9l4h3zg@public.gmane.org> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] wl12xx: fix size of two memset's in wl1271_cmd_build_arp_rsp()
From: Jesper Juhl @ 2012-04-22 20:53 UTC (permalink / raw)
To: linux-wireless, John W. Linville, Luciano Coelho; +Cc: netdev, linux-kernel
We currently do this:
int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, struct wl12xx_vif *wlvif)
...
struct wl12xx_arp_rsp_template *tmpl;
struct ieee80211_hdr_3addr *hdr;
...
tmpl = (struct wl12xx_arp_rsp_template *)skb_put(skb, sizeof(*tmpl));
memset(tmpl, 0, sizeof(tmpl));
...
hdr = (struct ieee80211_hdr_3addr *)skb_push(skb, sizeof(*hdr));
memset(hdr, 0, sizeof(*hdr));
...
I believe we want to set the entire structures to 0 with those
memset() calls, not just zero the initial part of them (size of the
pointer bytes).
Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
drivers/net/wireless/wl12xx/cmd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/cmd.c b/drivers/net/wireless/wl12xx/cmd.c
index 3414fc1..8e65529 100644
--- a/drivers/net/wireless/wl12xx/cmd.c
+++ b/drivers/net/wireless/wl12xx/cmd.c
@@ -1249,7 +1249,7 @@ int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, struct wl12xx_vif *wlvif)
skb_reserve(skb, sizeof(*hdr) + WL1271_EXTRA_SPACE_MAX);
tmpl = (struct wl12xx_arp_rsp_template *)skb_put(skb, sizeof(*tmpl));
- memset(tmpl, 0, sizeof(tmpl));
+ memset(tmpl, 0, sizeof(*tmpl));
/* llc layer */
memcpy(tmpl->llc_hdr, rfc1042_header, sizeof(rfc1042_header));
@@ -1298,7 +1298,7 @@ int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, struct wl12xx_vif *wlvif)
/* mac80211 header */
hdr = (struct ieee80211_hdr_3addr *)skb_push(skb, sizeof(*hdr));
- memset(hdr, 0, sizeof(hdr));
+ memset(hdr, 0, sizeof(*hdr));
fc = IEEE80211_FTYPE_DATA | IEEE80211_FCTL_TODS;
if (wlvif->sta.qos)
fc |= IEEE80211_STYPE_QOS_DATA;
--
1.7.10
Please CC me on replies.
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply related
* Re: [PATCH] tcp: fix TCP_MAXSEG for established IPv6 passive sockets
From: Eric Dumazet @ 2012-04-22 20:14 UTC (permalink / raw)
To: Neal Cardwell
Cc: David Miller, netdev, Eric Dumazet, Nandita Dukkipati,
Yuchung Cheng, ilpo.jarvinen, maze, Tom Herbert
In-Reply-To: <1335123947-9906-1-git-send-email-ncardwell@google.com>
On Sun, 2012-04-22 at 15:45 -0400, Neal Cardwell wrote:
> Commit f5fff5d forgot to fix TCP_MAXSEG behavior IPv6 sockets, so IPv6
> TCP server sockets that used TCP_MAXSEG would find that the advmss of
> child sockets would be incorrect. This commit mirrors the advmss logic
> from tcp_v4_syn_recv_sock in tcp_v6_syn_recv_sock. Eventually this
> logic should probably be shared between IPv4 and IPv6, but this at
> least fixes this issue.
>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> ---
> net/ipv6/tcp_ipv6.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 86cfe60..98256cf 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1383,6 +1383,10 @@ static struct sock * tcp_v6_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
> tcp_mtup_init(newsk);
> tcp_sync_mss(newsk, dst_mtu(dst));
> newtp->advmss = dst_metric_advmss(dst);
> + if (tcp_sk(sk)->rx_opt.user_mss &&
> + tcp_sk(sk)->rx_opt.user_mss < newtp->advmss)
> + newtp->advmss = tcp_sk(sk)->rx_opt.user_mss;
> +
> tcp_initialize_rcv_mss(newsk);
> if (tcp_rsk(req)->snt_synack)
> tcp_valid_rtt_meas(newsk,
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* [PATCH] tcp: fix TCP_MAXSEG for established IPv6 passive sockets
From: Neal Cardwell @ 2012-04-22 19:45 UTC (permalink / raw)
To: David Miller
Cc: netdev, Eric Dumazet, Nandita Dukkipati, Yuchung Cheng,
ilpo.jarvinen, maze, Tom Herbert, Neal Cardwell
Commit f5fff5d forgot to fix TCP_MAXSEG behavior IPv6 sockets, so IPv6
TCP server sockets that used TCP_MAXSEG would find that the advmss of
child sockets would be incorrect. This commit mirrors the advmss logic
from tcp_v4_syn_recv_sock in tcp_v6_syn_recv_sock. Eventually this
logic should probably be shared between IPv4 and IPv6, but this at
least fixes this issue.
Signed-off-by: Neal Cardwell <ncardwell@google.com>
---
net/ipv6/tcp_ipv6.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 86cfe60..98256cf 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1383,6 +1383,10 @@ static struct sock * tcp_v6_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
tcp_mtup_init(newsk);
tcp_sync_mss(newsk, dst_mtu(dst));
newtp->advmss = dst_metric_advmss(dst);
+ if (tcp_sk(sk)->rx_opt.user_mss &&
+ tcp_sk(sk)->rx_opt.user_mss < newtp->advmss)
+ newtp->advmss = tcp_sk(sk)->rx_opt.user_mss;
+
tcp_initialize_rcv_mss(newsk);
if (tcp_rsk(req)->snt_synack)
tcp_valid_rtt_meas(newsk,
--
1.7.7.3
^ permalink raw reply related
* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: Jamal Hadi Salim @ 2012-04-22 15:54 UTC (permalink / raw)
To: Stephen Hemminger
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
David Miller, eric dumazet
In-Reply-To: <61c89e02-c916-421e-b469-62b307853b1b-bX68f012229Xuxj3zoTs5AC/G2K4zDHf@public.gmane.org>
On Sun, 2012-04-22 at 08:22 -0700, Stephen Hemminger wrote:
> STT isn't really doing TCP, it just lying and pretending to be
> TCP to allow TSO to work! There is no packet ordering, sequence
> numbers or any real transport layer.
True. It is a nice engineering hack but even as a protocol enhancement
questionable at best.
> Therefore Simon's
> proposed hook is the only way to support it. But exposing that
> hook does allow for other misuse.
If you object to this, then you gotta object to the UDP equivalent
which has been around for sometime now for legitimate reasons
and could be used by STT (I think the claim was no hardware
does USO);->
cheers,
jamal
^ permalink raw reply
* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: Stephen Hemminger @ 2012-04-22 15:24 UTC (permalink / raw)
To: Simon Horman
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
Eric Dumazet
In-Reply-To: <20120419045333.GA21311-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
> This hook is based on a hook of the same name provided by UDP. It
> provides
> a way for to receive packets that have a TCP header and treat them in
> some
> alternate way.
>
> It is intended to be used by an implementation of the STT tunneling
> protocol within Open vSwtich's datapath. A prototype of such an
> implementation has been made.
>
> The STT draft is available at
> http://tools.ietf.org/html/draft-davie-stt-01
>
> My prototype STT implementation has been posted to the
> dev-UOEtcQmXneFl884UGnbwIQ@public.gmane.org
> The second version can be found at:
> http://www.mail-archive.com/dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org/msg09001.html
> It needs to be updated to call tcp_encap_enable()
>
> Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
>
>
> +static struct static_key tcp_encap_needed __read_mostly;
> +void tcp_encap_enable(void)
> +{
> + if (!static_key_enabled(&tcp_encap_needed))
> + static_key_slow_inc(&tcp_encap_needed);
> +}
> +EXPORT_SYMBOL(tcp_encap_enable);
I have reservations about adding such a hook. but if we
must, then the hook must be GPL only.
^ permalink raw reply
* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: Stephen Hemminger @ 2012-04-22 15:22 UTC (permalink / raw)
To: David Miller
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
eric dumazet
In-Reply-To: <20120421.153743.699070106218049860.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> From: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
> Date: Thu, 19 Apr 2012 13:53:35 +0900
>
> > This hook is based on a hook of the same name provided by UDP. It
> > provides
> > a way for to receive packets that have a TCP header and treat them
> > in some
> > alternate way.
> >
> > It is intended to be used by an implementation of the STT tunneling
> > protocol within Open vSwtich's datapath. A prototype of such an
> > implementation has been made.
> >
> > The STT draft is available at
> > http://tools.ietf.org/html/draft-davie-stt-01
>
> I think that unlike UDP, you need to let the stack handle the TCP
> packet before taking it into your subsystem. The reason is that
> otherwise you'll need to handle packet ordering, sequence number
> gaps,
> and proper TCP ACK'ing and timeout all inside of your module and
> that's simply unacceptable.
STT isn't really doing TCP, it just lying and pretending to be
TCP to allow TSO to work! There is no packet ordering, sequence
numbers or any real transport layer. Therefore Simon's
proposed hook is the only way to support it. But exposing that
hook does allow for other misuse.
^ permalink raw reply
* Re: Question with secure_ipv4_port_ephemeral() implementation
From: Tetsuo Handa @ 2012-04-22 14:38 UTC (permalink / raw)
To: bhutchings; +Cc: netdev
In-Reply-To: <1335104462.3209.377.camel@deadeye>
Ben Hutchings wrote:
> All this randomisation is concerned with preventing spoofing attacks by
> attackers that can't see any packets routed to the spoofed address. If
> they can see the return packets then this is all in vain, which is why
> we generally want cryptography at the transport or application level.
> But if they can't then the port offset remains secret - right?
>
> (Port randomisation is not very useful on it own due to the small number
> space, but see
> <http://en.wikipedia.org/wiki/DNS_cache_poisoning#Prevention_and_mitigation>.)
Thank you. So, this change (which looked to me a regression) is not a regression.
^ permalink raw reply
* Re: Question with secure_ipv4_port_ephemeral() implementation
From: Ben Hutchings @ 2012-04-22 14:21 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: netdev
In-Reply-To: <201204221420.GJB51054.OtHOFSQJFOVMFL@I-love.SAKURA.ne.jp>
On Sun, 2012-04-22 at 14:20 +0900, Tetsuo Handa wrote:
> Ben Hutchings wrote:
> > As I understand it, that 8-bit counter was used for all connections, so
> > in order to spoof the source of a TCP connection it was only necessary
> > to guess 24 bits of the ISN. On a sufficiently fast network, it would
> > now be feasible to carry out a brute force attack that ACKs all possible
> > ISNs before the handshake times-out. That's not yet feasible if the
> > attacker has to guess all 32 bits of the ISN.
>
> So, the purpose was to make the initial sequence number more random. OK.
>
> > The original reason for periodically regenerating the secret was that
> > the hash function was quite weak and the secret could presumably be
> > found in a reasonably short time. So, without regeneration, the hash
> > also has to be stronger.
>
> My concern is the purpose of making the automatic local port number selection
> algorithm less random. That commit removed uptime factor from factors that
> determine starting point of available local port scanning (due to removal of
> periodic get_random_bytes() calls).
>
> 368 static inline u32 inet_sk_port_offset(const struct sock *sk)
> 369 {
> 370 const struct inet_sock *inet = inet_sk(sk);
> 371 return secure_ipv4_port_ephemeral(inet->inet_rcv_saddr,
> 372 inet->inet_daddr,
> 373 inet->inet_dport);
> 374 }
>
> secure_ipv4_port_ephemeral() no longer depends on uptime.
>
> 565 int inet_hash_connect(struct inet_timewait_death_row *death_row,
> 566 struct sock *sk)
> 567 {
> 568 return __inet_hash_connect(death_row, sk, inet_sk_port_offset(sk),
> 569 __inet_check_established, __inet_hash_nolisten);
> 570 }
>
> inet_sk_port_offset() no longer depends on uptime.
> It returns same port offset for same addresses.
[...]
All this randomisation is concerned with preventing spoofing attacks by
attackers that can't see any packets routed to the spoofed address. If
they can see the return packets then this is all in vain, which is why
we generally want cryptography at the transport or application level.
But if they can't then the port offset remains secret - right?
(Port randomisation is not very useful on it own due to the small number
space, but see
<http://en.wikipedia.org/wiki/DNS_cache_poisoning#Prevention_and_mitigation>.)
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: use-after-free in usbnet
From: Ming Lei @ 2012-04-22 14:19 UTC (permalink / raw)
To: Huajun Li
Cc: Oliver Neukum, Alan Stern, Dave Jones, netdev, linux-usb,
Fedora Kernel Team
In-Reply-To: <CA+v9cxZfzATWynXGtEe6gvcD2aRsLqAF3ZN_HM_dyU4W_rQSpg@mail.gmail.com>
On Sun, Apr 22, 2012 at 9:15 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
> On Sun, Apr 22, 2012 at 8:05 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>> On Sun, Apr 22, 2012 at 10:19 AM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>>> On Sat, Apr 21, 2012 at 3:45 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>>>> Hi Huajun,
>>>>
>>>> On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>>>>>> Just skip trying this per your following email's comments.
>>>>>
>>>>> I will prepare a new patch later, if you'd like to try it.
>>>>
>>>> The below patch reverts the below commits:
>>>>
>>>> 0956a8c20b23d429e79ff86d4325583fc06f9eb4
>>>> (usbnet: increase URB reference count before usb_unlink_urb)
>>>>
>>>> 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d
>>>> (net/usbnet: avoid recursive locking in usbnet_stop())
>>>>
>>>> and keep holding tx/rx queue lock during unlinking, but avoid
>>>> to acquire the same queue lock inside complete handler triggered by
>>>> usb_unlink_urb.
>>>>
>>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>>> index db99536..effb34e 100644
>>>> --- a/drivers/net/usb/usbnet.c
>>>> +++ b/drivers/net/usb/usbnet.c
>>>> @@ -291,9 +291,11 @@ static void defer_bh(struct usbnet *dev, struct
>>>> sk_buff *skb, struct sk_buff_hea
>>>> {
>>>> unsigned long flags;
>>>>
>>>> - spin_lock_irqsave(&list->lock, flags);
>>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>>> + spin_lock_irqsave(&list->lock, flags);
>>>> __skb_unlink(skb, list);
>>>> - spin_unlock(&list->lock);
>>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>>> + spin_unlock(&list->lock);
>>>> spin_lock(&dev->done.lock);
>>>> __skb_queue_tail(&dev->done, skb);
>>>> if (dev->done.qlen == 1)
>>>
>>>
>>> Then 'flags' may not be initialized, and this will cause problem while
>>> calling spin_unlock_irqrestore(&dev->done.lock, flags), right?
>>
>> The flag is a percpu variable, so it can't change during the
>> above code piece.
>>
>
> No, maybe you misunderstood it.
> Legacy code stack has 'flags' variable to save irq information, and at
> the end defer_bh(), spin_unlock_irqrestore(&dev->done.lock, flags)
> will use it. However, the variable may not be initialized in your
> patch.
Got it, it is easy to fix it by replace the below line:
spin_unlock_irqrestore(&dev->done.lock, flags);
with these:
if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
spin_unlock_irqrestore(&dev->done.lock, flags);
else
spin_unlock(&dev->done.lock);
>
>>>
>>>
>>>> @@ -345,7 +347,8 @@ static int rx_submit (struct usbnet *dev, struct
>>>> urb *urb, gfp_t flags)
>>>> usb_fill_bulk_urb (urb, dev->udev, dev->in,
>>>> skb->data, size, rx_complete, skb);
>>>>
>>>> - spin_lock_irqsave (&dev->rxq.lock, lockflags);
>>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>>> + spin_lock_irqsave (&dev->rxq.lock, lockflags);
>>>>
>>>> if (netif_running (dev->net) &&
>>>> netif_device_present (dev->net) &&
>>>> @@ -377,7 +380,8 @@ static int rx_submit (struct usbnet *dev, struct
>>>> urb *urb, gfp_t flags)
>>>> netif_dbg(dev, ifdown, dev->net, "rx: stopped\n");
>>>> retval = -ENOLINK;
>>>> }
>>>> - spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
>>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>>> + spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
>>>> if (retval) {
>>>> dev_kfree_skb_any (skb);
>>>> usb_free_urb (urb);
>>>> @@ -582,6 +586,7 @@ static int unlink_urbs (struct usbnet *dev, struct
>>>> sk_buff_head *q)
>>>> int count = 0;
>>>>
>>>> spin_lock_irqsave (&q->lock, flags);
>>>> + set_cpu_bit(URB_UNLINKING, dev->cpuflags);
>>>> skb_queue_walk_safe(q, skb, skbnext) {
>>>> struct skb_data *entry;
>>>> struct urb *urb;
>>>> @@ -590,15 +595,6 @@ static int unlink_urbs (struct usbnet *dev,
>>>> struct sk_buff_head *q)
>>>> entry = (struct skb_data *) skb->cb;
>>>> urb = entry->urb;
>>>>
>>>> - /*
>>>> - * Get reference count of the URB to avoid it to be
>>>> - * freed during usb_unlink_urb, which may trigger
>>>> - * use-after-free problem inside usb_unlink_urb since
>>>> - * usb_unlink_urb is always racing with .complete
>>>> - * handler(include defer_bh).
>>>> - */
>>>> - usb_get_urb(urb);
>>>> - spin_unlock_irqrestore(&q->lock, flags);
>>>> // during some PM-driven resume scenarios,
>>>> // these (async) unlinks complete immediately
>>>> retval = usb_unlink_urb (urb);
>>>> @@ -606,9 +602,8 @@ static int unlink_urbs (struct usbnet *dev, struct
>>>> sk_buff_head *q)
>>>> netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
>>>> else
>>>> count++;
>>>> - usb_put_urb(urb);
>>>> - spin_lock_irqsave(&q->lock, flags);
>>>> }
>>>> + clear_cpu_bit(URB_UNLINKING, dev->cpuflags);
>>>> spin_unlock_irqrestore (&q->lock, flags);
>>>> return count;
>>>> }
>>>> @@ -1283,6 +1278,7 @@ void usbnet_disconnect (struct usb_interface *intf)
>>>> usb_kill_urb(dev->interrupt);
>>>> usb_free_urb(dev->interrupt);
>>>>
>>>> + free_percpu(dev->cpuflags);
>>>> free_netdev(net);
>>>> usb_put_dev (xdev);
>>>> }
>>>> @@ -1353,6 +1349,13 @@ usbnet_probe (struct usb_interface *udev, const
>>>> struct usb_device_id *prod)
>>>> SET_NETDEV_DEV(net, &udev->dev);
>>>>
>>>> dev = netdev_priv(net);
>>>> +
>>>> + dev->cpuflags = alloc_percpu(unsigned long);
>>>> + if (!dev->cpuflags) {
>>>> + status = -ENOMEM;
>>>> + goto out1;
>>>> + }
>>>> +
>>>> dev->udev = xdev;
>>>> dev->intf = udev;
>>>> dev->driver_info = info;
>>>> @@ -1396,7 +1399,7 @@ usbnet_probe (struct usb_interface *udev, const
>>>> struct usb_device_id *prod)
>>>> if (info->bind) {
>>>> status = info->bind (dev, udev);
>>>> if (status < 0)
>>>> - goto out1;
>>>> + goto out2;
>>>>
>>>> // heuristic: "usb%d" for links we know are two-host,
>>>> // else "eth%d" when there's reasonable doubt. userspace
>>>> @@ -1465,6 +1468,8 @@ usbnet_probe (struct usb_interface *udev, const
>>>> struct usb_device_id *prod)
>>>> out3:
>>>> if (info->unbind)
>>>> info->unbind (dev, udev);
>>>> +out2:
>>>> + free_percpu(dev->cpuflags);
>>>> out1:
>>>> free_netdev(net);
>>>> out:
>>>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
>>>> index 605b0aa..2dc46f5 100644
>>>> --- a/include/linux/usb/usbnet.h
>>>> +++ b/include/linux/usb/usbnet.h
>>>> @@ -69,8 +69,28 @@ struct usbnet {
>>>> # define EVENT_DEV_WAKING 6
>>>> # define EVENT_DEV_ASLEEP 7
>>>> # define EVENT_DEV_OPEN 8
>>>> + unsigned long __percpu *cpuflags;
>>>> +# define URB_UNLINKING 0
>>>
>>> Is it possible using a simple bool variable to track whether q->lock
>>> is hold by unlink_urb() ? If yes, it can avoid introducing following
>>> new codes into current code stack.
>>
>> It should be defined as percpu variable. The URB complete handler
>> may be triggered inside unlink path, or it can be triggered in hardirq path
>> from other CPUs at the same time.
>>
>
> Then the potential issue is, in defer_bh(), __skb_unlink() may be
> called without holding the lock. This can cause current issue too,
No, URB_UNLINKING being set means the lock has been held during unlink,
see unlink_urbs related changes in the percpu flag patch.
> because __skb_unlink() may move 'skb->next' out of txq/rxq, and even
> free it.
>
>>>
>>>> };
>>>>
>>>> +static inline void set_cpu_bit(int nr, unsigned long __percpu *addr)
>>>> +{
>>>> + unsigned long *fl = __this_cpu_ptr(addr);
>>>> + set_bit(nr, fl);
>>>> +}
>>>> +
>>>> +static inline void clear_cpu_bit(int nr, unsigned long __percpu *addr)
>>>> +{
>>>> + unsigned long *fl = __this_cpu_ptr(addr);
>>>> + clear_bit(nr, fl);
>>>> +}
>>>> +
>>>> +static inline int test_cpu_bit(int nr, unsigned long __percpu *addr)
>>>> +{
>>>> + unsigned long *fl = __this_cpu_ptr(addr);
>>>> + return test_bit(nr, fl);
>>>> +}
>>>> +
>>>> static inline struct usb_driver *driver_of(struct usb_interface *intf)
>>>> {
>>>> return to_usb_driver(intf->dev.driver);
>>>>
>>>>
>>>> Thanks,
>>>> --
>>>> Ming Lei
>>
>>
>>
>> --
>> Ming Lei
--
Ming Lei
^ permalink raw reply
* Re: use-after-free in usbnet
From: Ming Lei @ 2012-04-22 14:15 UTC (permalink / raw)
To: Huajun Li
Cc: Oliver Neukum, Alan Stern, Dave Jones,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Fedora Kernel Team
In-Reply-To: <CA+v9cxZfzATWynXGtEe6gvcD2aRsLqAF3ZN_HM_dyU4W_rQSpg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Sun, Apr 22, 2012 at 9:15 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sun, Apr 22, 2012 at 8:05 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Sun, Apr 22, 2012 at 10:19 AM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On Sat, Apr 21, 2012 at 3:45 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> Hi Huajun,
>>>>
>>>> On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>> Just skip trying this per your following email's comments.
>>>>>
>>>>> I will prepare a new patch later, if you'd like to try it.
>>>>
>>>> The below patch reverts the below commits:
>>>>
>>>> 0956a8c20b23d429e79ff86d4325583fc06f9eb4
>>>> (usbnet: increase URB reference count before usb_unlink_urb)
>>>>
>>>> 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d
>>>> (net/usbnet: avoid recursive locking in usbnet_stop())
>>>>
>>>> and keep holding tx/rx queue lock during unlinking, but avoid
>>>> to acquire the same queue lock inside complete handler triggered by
>>>> usb_unlink_urb.
>>>>
>>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>>> index db99536..effb34e 100644
>>>> --- a/drivers/net/usb/usbnet.c
>>>> +++ b/drivers/net/usb/usbnet.c
>>>> @@ -291,9 +291,11 @@ static void defer_bh(struct usbnet *dev, struct
>>>> sk_buff *skb, struct sk_buff_hea
>>>> {
>>>> unsigned long flags;
>>>>
>>>> - spin_lock_irqsave(&list->lock, flags);
>>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>>> + spin_lock_irqsave(&list->lock, flags);
>>>> __skb_unlink(skb, list);
>>>> - spin_unlock(&list->lock);
>>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>>> + spin_unlock(&list->lock);
>>>> spin_lock(&dev->done.lock);
>>>> __skb_queue_tail(&dev->done, skb);
>>>> if (dev->done.qlen == 1)
>>>
>>>
>>> Then 'flags' may not be initialized, and this will cause problem while
>>> calling spin_unlock_irqrestore(&dev->done.lock, flags), right?
>>
>> The flag is a percpu variable, so it can't change during the
>> above code piece.
>>
>
> No, maybe you misunderstood it.
> Legacy code stack has 'flags' variable to save irq information, and at
> the end defer_bh(), spin_unlock_irqrestore(&dev->done.lock, flags)
> will use it. However, the variable may not be initialized in your
> patch.
Got it, it is easy to fix it by replace the below line:
spin_unlock_irqrestore(&dev->done.lock, flags);
with these:
if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
spin_unlock_irqrestore(&dev->done.lock, flags);
else
spin_unlock(&dev->done.lock);
>
>>>
>>>
>>>> @@ -345,7 +347,8 @@ static int rx_submit (struct usbnet *dev, struct
>>>> urb *urb, gfp_t flags)
>>>> usb_fill_bulk_urb (urb, dev->udev, dev->in,
>>>> skb->data, size, rx_complete, skb);
>>>>
>>>> - spin_lock_irqsave (&dev->rxq.lock, lockflags);
>>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>>> + spin_lock_irqsave (&dev->rxq.lock, lockflags);
>>>>
>>>> if (netif_running (dev->net) &&
>>>> netif_device_present (dev->net) &&
>>>> @@ -377,7 +380,8 @@ static int rx_submit (struct usbnet *dev, struct
>>>> urb *urb, gfp_t flags)
>>>> netif_dbg(dev, ifdown, dev->net, "rx: stopped\n");
>>>> retval = -ENOLINK;
>>>> }
>>>> - spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
>>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>>> + spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
>>>> if (retval) {
>>>> dev_kfree_skb_any (skb);
>>>> usb_free_urb (urb);
>>>> @@ -582,6 +586,7 @@ static int unlink_urbs (struct usbnet *dev, struct
>>>> sk_buff_head *q)
>>>> int count = 0;
>>>>
>>>> spin_lock_irqsave (&q->lock, flags);
>>>> + set_cpu_bit(URB_UNLINKING, dev->cpuflags);
>>>> skb_queue_walk_safe(q, skb, skbnext) {
>>>> struct skb_data *entry;
>>>> struct urb *urb;
>>>> @@ -590,15 +595,6 @@ static int unlink_urbs (struct usbnet *dev,
>>>> struct sk_buff_head *q)
>>>> entry = (struct skb_data *) skb->cb;
>>>> urb = entry->urb;
>>>>
>>>> - /*
>>>> - * Get reference count of the URB to avoid it to be
>>>> - * freed during usb_unlink_urb, which may trigger
>>>> - * use-after-free problem inside usb_unlink_urb since
>>>> - * usb_unlink_urb is always racing with .complete
>>>> - * handler(include defer_bh).
>>>> - */
>>>> - usb_get_urb(urb);
>>>> - spin_unlock_irqrestore(&q->lock, flags);
>>>> // during some PM-driven resume scenarios,
>>>> // these (async) unlinks complete immediately
>>>> retval = usb_unlink_urb (urb);
>>>> @@ -606,9 +602,8 @@ static int unlink_urbs (struct usbnet *dev, struct
>>>> sk_buff_head *q)
>>>> netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
>>>> else
>>>> count++;
>>>> - usb_put_urb(urb);
>>>> - spin_lock_irqsave(&q->lock, flags);
>>>> }
>>>> + clear_cpu_bit(URB_UNLINKING, dev->cpuflags);
>>>> spin_unlock_irqrestore (&q->lock, flags);
>>>> return count;
>>>> }
>>>> @@ -1283,6 +1278,7 @@ void usbnet_disconnect (struct usb_interface *intf)
>>>> usb_kill_urb(dev->interrupt);
>>>> usb_free_urb(dev->interrupt);
>>>>
>>>> + free_percpu(dev->cpuflags);
>>>> free_netdev(net);
>>>> usb_put_dev (xdev);
>>>> }
>>>> @@ -1353,6 +1349,13 @@ usbnet_probe (struct usb_interface *udev, const
>>>> struct usb_device_id *prod)
>>>> SET_NETDEV_DEV(net, &udev->dev);
>>>>
>>>> dev = netdev_priv(net);
>>>> +
>>>> + dev->cpuflags = alloc_percpu(unsigned long);
>>>> + if (!dev->cpuflags) {
>>>> + status = -ENOMEM;
>>>> + goto out1;
>>>> + }
>>>> +
>>>> dev->udev = xdev;
>>>> dev->intf = udev;
>>>> dev->driver_info = info;
>>>> @@ -1396,7 +1399,7 @@ usbnet_probe (struct usb_interface *udev, const
>>>> struct usb_device_id *prod)
>>>> if (info->bind) {
>>>> status = info->bind (dev, udev);
>>>> if (status < 0)
>>>> - goto out1;
>>>> + goto out2;
>>>>
>>>> // heuristic: "usb%d" for links we know are two-host,
>>>> // else "eth%d" when there's reasonable doubt. userspace
>>>> @@ -1465,6 +1468,8 @@ usbnet_probe (struct usb_interface *udev, const
>>>> struct usb_device_id *prod)
>>>> out3:
>>>> if (info->unbind)
>>>> info->unbind (dev, udev);
>>>> +out2:
>>>> + free_percpu(dev->cpuflags);
>>>> out1:
>>>> free_netdev(net);
>>>> out:
>>>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
>>>> index 605b0aa..2dc46f5 100644
>>>> --- a/include/linux/usb/usbnet.h
>>>> +++ b/include/linux/usb/usbnet.h
>>>> @@ -69,8 +69,28 @@ struct usbnet {
>>>> # define EVENT_DEV_WAKING 6
>>>> # define EVENT_DEV_ASLEEP 7
>>>> # define EVENT_DEV_OPEN 8
>>>> + unsigned long __percpu *cpuflags;
>>>> +# define URB_UNLINKING 0
>>>
>>> Is it possible using a simple bool variable to track whether q->lock
>>> is hold by unlink_urb() ? If yes, it can avoid introducing following
>>> new codes into current code stack.
>>
>> It should be defined as percpu variable. The URB complete handler
>> may be triggered inside unlink path, or it can be triggered in hardirq path
>> from other CPUs at the same time.
>>
>
> Then the potential issue is, in defer_bh(), __skb_unlink() may be
> called without holding the lock. This can cause current issue too,
No, URB_UNLINKING being set means the lock has been held during unlink,
see unlink_urbs related changes in the percpu flag patch.
> because __skb_unlink() may move 'skb->next' out of txq/rxq, and even
> free it.
>
>>>
>>>> };
>>>>
>>>> +static inline void set_cpu_bit(int nr, unsigned long __percpu *addr)
>>>> +{
>>>> + unsigned long *fl = __this_cpu_ptr(addr);
>>>> + set_bit(nr, fl);
>>>> +}
>>>> +
>>>> +static inline void clear_cpu_bit(int nr, unsigned long __percpu *addr)
>>>> +{
>>>> + unsigned long *fl = __this_cpu_ptr(addr);
>>>> + clear_bit(nr, fl);
>>>> +}
>>>> +
>>>> +static inline int test_cpu_bit(int nr, unsigned long __percpu *addr)
>>>> +{
>>>> + unsigned long *fl = __this_cpu_ptr(addr);
>>>> + return test_bit(nr, fl);
>>>> +}
>>>> +
>>>> static inline struct usb_driver *driver_of(struct usb_interface *intf)
>>>> {
>>>> return to_usb_driver(intf->dev.driver);
>>>>
>>>>
>>>> Thanks,
>>>> --
>>>> Ming Lei
>>
>>
>>
>> --
>> Ming Lei
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: use-after-free in usbnet
From: Ming Lei @ 2012-04-22 14:10 UTC (permalink / raw)
To: Huajun Li
Cc: Oliver Neukum, Alan Stern, Dave Jones,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Fedora Kernel Team
In-Reply-To: <CA+v9cxZfzATWynXGtEe6gvcD2aRsLqAF3ZN_HM_dyU4W_rQSpg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Sun, Apr 22, 2012 at 9:15 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sun, Apr 22, 2012 at 8:05 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Sun, Apr 22, 2012 at 10:19 AM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On Sat, Apr 21, 2012 at 3:45 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> Hi Huajun,
>>>>
>>>> On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>> Just skip trying this per your following email's comments.
>>>>>
>>>>> I will prepare a new patch later, if you'd like to try it.
>>>>
>>>> The below patch reverts the below commits:
>>>>
>>>> 0956a8c20b23d429e79ff86d4325583fc06f9eb4
>>>> (usbnet: increase URB reference count before usb_unlink_urb)
>>>>
>>>> 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d
>>>> (net/usbnet: avoid recursive locking in usbnet_stop())
>>>>
>>>> and keep holding tx/rx queue lock during unlinking, but avoid
>>>> to acquire the same queue lock inside complete handler triggered by
>>>> usb_unlink_urb.
>>>>
>>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>>> index db99536..effb34e 100644
>>>> --- a/drivers/net/usb/usbnet.c
>>>> +++ b/drivers/net/usb/usbnet.c
>>>> @@ -291,9 +291,11 @@ static void defer_bh(struct usbnet *dev, struct
>>>> sk_buff *skb, struct sk_buff_hea
>>>> {
>>>> unsigned long flags;
>>>>
>>>> - spin_lock_irqsave(&list->lock, flags);
>>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>>> + spin_lock_irqsave(&list->lock, flags);
>>>> __skb_unlink(skb, list);
>>>> - spin_unlock(&list->lock);
>>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>>> + spin_unlock(&list->lock);
>>>> spin_lock(&dev->done.lock);
>>>> __skb_queue_tail(&dev->done, skb);
>>>> if (dev->done.qlen == 1)
>>>
>>>
>>> Then 'flags' may not be initialized, and this will cause problem while
>>> calling spin_unlock_irqrestore(&dev->done.lock, flags), right?
>>
>> The flag is a percpu variable, so it can't change during the
>> above code piece.
>>
>
> No, maybe you misunderstood it.
> Legacy code stack has 'flags' variable to save irq information, and at
> the end defer_bh(), spin_unlock_irqrestore(&dev->done.lock, flags)
> will use it. However, the variable may not be initialized in your
> patch.
Got it, it is easy to fix it by replace the below line:
spin_unlock_irqrestore(&dev->done.lock, flags);
with these:
if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
spin_unlock_irqrestore(&dev->done.lock, flags);
else
spin_unlock(&dev->done.lock);
>
>>>
>>>
>>>> @@ -345,7 +347,8 @@ static int rx_submit (struct usbnet *dev, struct
>>>> urb *urb, gfp_t flags)
>>>> usb_fill_bulk_urb (urb, dev->udev, dev->in,
>>>> skb->data, size, rx_complete, skb);
>>>>
>>>> - spin_lock_irqsave (&dev->rxq.lock, lockflags);
>>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>>> + spin_lock_irqsave (&dev->rxq.lock, lockflags);
>>>>
>>>> if (netif_running (dev->net) &&
>>>> netif_device_present (dev->net) &&
>>>> @@ -377,7 +380,8 @@ static int rx_submit (struct usbnet *dev, struct
>>>> urb *urb, gfp_t flags)
>>>> netif_dbg(dev, ifdown, dev->net, "rx: stopped\n");
>>>> retval = -ENOLINK;
>>>> }
>>>> - spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
>>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>>> + spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
>>>> if (retval) {
>>>> dev_kfree_skb_any (skb);
>>>> usb_free_urb (urb);
>>>> @@ -582,6 +586,7 @@ static int unlink_urbs (struct usbnet *dev, struct
>>>> sk_buff_head *q)
>>>> int count = 0;
>>>>
>>>> spin_lock_irqsave (&q->lock, flags);
>>>> + set_cpu_bit(URB_UNLINKING, dev->cpuflags);
>>>> skb_queue_walk_safe(q, skb, skbnext) {
>>>> struct skb_data *entry;
>>>> struct urb *urb;
>>>> @@ -590,15 +595,6 @@ static int unlink_urbs (struct usbnet *dev,
>>>> struct sk_buff_head *q)
>>>> entry = (struct skb_data *) skb->cb;
>>>> urb = entry->urb;
>>>>
>>>> - /*
>>>> - * Get reference count of the URB to avoid it to be
>>>> - * freed during usb_unlink_urb, which may trigger
>>>> - * use-after-free problem inside usb_unlink_urb since
>>>> - * usb_unlink_urb is always racing with .complete
>>>> - * handler(include defer_bh).
>>>> - */
>>>> - usb_get_urb(urb);
>>>> - spin_unlock_irqrestore(&q->lock, flags);
>>>> // during some PM-driven resume scenarios,
>>>> // these (async) unlinks complete immediately
>>>> retval = usb_unlink_urb (urb);
>>>> @@ -606,9 +602,8 @@ static int unlink_urbs (struct usbnet *dev, struct
>>>> sk_buff_head *q)
>>>> netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
>>>> else
>>>> count++;
>>>> - usb_put_urb(urb);
>>>> - spin_lock_irqsave(&q->lock, flags);
>>>> }
>>>> + clear_cpu_bit(URB_UNLINKING, dev->cpuflags);
>>>> spin_unlock_irqrestore (&q->lock, flags);
>>>> return count;
>>>> }
>>>> @@ -1283,6 +1278,7 @@ void usbnet_disconnect (struct usb_interface *intf)
>>>> usb_kill_urb(dev->interrupt);
>>>> usb_free_urb(dev->interrupt);
>>>>
>>>> + free_percpu(dev->cpuflags);
>>>> free_netdev(net);
>>>> usb_put_dev (xdev);
>>>> }
>>>> @@ -1353,6 +1349,13 @@ usbnet_probe (struct usb_interface *udev, const
>>>> struct usb_device_id *prod)
>>>> SET_NETDEV_DEV(net, &udev->dev);
>>>>
>>>> dev = netdev_priv(net);
>>>> +
>>>> + dev->cpuflags = alloc_percpu(unsigned long);
>>>> + if (!dev->cpuflags) {
>>>> + status = -ENOMEM;
>>>> + goto out1;
>>>> + }
>>>> +
>>>> dev->udev = xdev;
>>>> dev->intf = udev;
>>>> dev->driver_info = info;
>>>> @@ -1396,7 +1399,7 @@ usbnet_probe (struct usb_interface *udev, const
>>>> struct usb_device_id *prod)
>>>> if (info->bind) {
>>>> status = info->bind (dev, udev);
>>>> if (status < 0)
>>>> - goto out1;
>>>> + goto out2;
>>>>
>>>> // heuristic: "usb%d" for links we know are two-host,
>>>> // else "eth%d" when there's reasonable doubt. userspace
>>>> @@ -1465,6 +1468,8 @@ usbnet_probe (struct usb_interface *udev, const
>>>> struct usb_device_id *prod)
>>>> out3:
>>>> if (info->unbind)
>>>> info->unbind (dev, udev);
>>>> +out2:
>>>> + free_percpu(dev->cpuflags);
>>>> out1:
>>>> free_netdev(net);
>>>> out:
>>>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
>>>> index 605b0aa..2dc46f5 100644
>>>> --- a/include/linux/usb/usbnet.h
>>>> +++ b/include/linux/usb/usbnet.h
>>>> @@ -69,8 +69,28 @@ struct usbnet {
>>>> # define EVENT_DEV_WAKING 6
>>>> # define EVENT_DEV_ASLEEP 7
>>>> # define EVENT_DEV_OPEN 8
>>>> + unsigned long __percpu *cpuflags;
>>>> +# define URB_UNLINKING 0
>>>
>>> Is it possible using a simple bool variable to track whether q->lock
>>> is hold by unlink_urb() ? If yes, it can avoid introducing following
>>> new codes into current code stack.
>>
>> It should be defined as percpu variable. The URB complete handler
>> may be triggered inside unlink path, or it can be triggered in hardirq path
>> from other CPUs at the same time.
>>
>
> Then the potential issue is, in defer_bh(), __skb_unlink() may be
> called without holding the lock. This can cause current issue too,
No, URB_UNLINKING being set means the lock has been held during unlink,
see unlink_urbs related changes in the percpu flag patch.
> because __skb_unlink() may move 'skb->next' out of txq/rxq, and even
> free it.
>
>>>
>>>> };
>>>>
>>>> +static inline void set_cpu_bit(int nr, unsigned long __percpu *addr)
>>>> +{
>>>> + unsigned long *fl = __this_cpu_ptr(addr);
>>>> + set_bit(nr, fl);
>>>> +}
>>>> +
>>>> +static inline void clear_cpu_bit(int nr, unsigned long __percpu *addr)
>>>> +{
>>>> + unsigned long *fl = __this_cpu_ptr(addr);
>>>> + clear_bit(nr, fl);
>>>> +}
>>>> +
>>>> +static inline int test_cpu_bit(int nr, unsigned long __percpu *addr)
>>>> +{
>>>> + unsigned long *fl = __this_cpu_ptr(addr);
>>>> + return test_bit(nr, fl);
>>>> +}
>>>> +
>>>> static inline struct usb_driver *driver_of(struct usb_interface *intf)
>>>> {
>>>> return to_usb_driver(intf->dev.driver);
>>>>
>>>>
>>>> Thanks,
>>>> --
>>>> Ming Lei
>>
>>
>>
>> --
>> Ming Lei
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: use-after-free in usbnet
From: Huajun Li @ 2012-04-22 13:15 UTC (permalink / raw)
To: Ming Lei
Cc: Oliver Neukum, Alan Stern, Dave Jones, netdev, linux-usb,
Fedora Kernel Team
In-Reply-To: <CACVXFVMZKBm_=wL=-T=7z8hc+KGHHSNsoRGwHOL4gZtfsZJ46g@mail.gmail.com>
On Sun, Apr 22, 2012 at 8:05 PM, Ming Lei <tom.leiming@gmail.com> wrote:
> On Sun, Apr 22, 2012 at 10:19 AM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>> On Sat, Apr 21, 2012 at 3:45 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>>> Hi Huajun,
>>>
>>> On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>>>>> Just skip trying this per your following email's comments.
>>>>
>>>> I will prepare a new patch later, if you'd like to try it.
>>>
>>> The below patch reverts the below commits:
>>>
>>> 0956a8c20b23d429e79ff86d4325583fc06f9eb4
>>> (usbnet: increase URB reference count before usb_unlink_urb)
>>>
>>> 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d
>>> (net/usbnet: avoid recursive locking in usbnet_stop())
>>>
>>> and keep holding tx/rx queue lock during unlinking, but avoid
>>> to acquire the same queue lock inside complete handler triggered by
>>> usb_unlink_urb.
>>>
>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>> index db99536..effb34e 100644
>>> --- a/drivers/net/usb/usbnet.c
>>> +++ b/drivers/net/usb/usbnet.c
>>> @@ -291,9 +291,11 @@ static void defer_bh(struct usbnet *dev, struct
>>> sk_buff *skb, struct sk_buff_hea
>>> {
>>> unsigned long flags;
>>>
>>> - spin_lock_irqsave(&list->lock, flags);
>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>> + spin_lock_irqsave(&list->lock, flags);
>>> __skb_unlink(skb, list);
>>> - spin_unlock(&list->lock);
>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>> + spin_unlock(&list->lock);
>>> spin_lock(&dev->done.lock);
>>> __skb_queue_tail(&dev->done, skb);
>>> if (dev->done.qlen == 1)
>>
>>
>> Then 'flags' may not be initialized, and this will cause problem while
>> calling spin_unlock_irqrestore(&dev->done.lock, flags), right?
>
> The flag is a percpu variable, so it can't change during the
> above code piece.
>
No, maybe you misunderstood it.
Legacy code stack has 'flags' variable to save irq information, and at
the end defer_bh(), spin_unlock_irqrestore(&dev->done.lock, flags)
will use it. However, the variable may not be initialized in your
patch.
>>
>>
>>> @@ -345,7 +347,8 @@ static int rx_submit (struct usbnet *dev, struct
>>> urb *urb, gfp_t flags)
>>> usb_fill_bulk_urb (urb, dev->udev, dev->in,
>>> skb->data, size, rx_complete, skb);
>>>
>>> - spin_lock_irqsave (&dev->rxq.lock, lockflags);
>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>> + spin_lock_irqsave (&dev->rxq.lock, lockflags);
>>>
>>> if (netif_running (dev->net) &&
>>> netif_device_present (dev->net) &&
>>> @@ -377,7 +380,8 @@ static int rx_submit (struct usbnet *dev, struct
>>> urb *urb, gfp_t flags)
>>> netif_dbg(dev, ifdown, dev->net, "rx: stopped\n");
>>> retval = -ENOLINK;
>>> }
>>> - spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
>>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>> + spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
>>> if (retval) {
>>> dev_kfree_skb_any (skb);
>>> usb_free_urb (urb);
>>> @@ -582,6 +586,7 @@ static int unlink_urbs (struct usbnet *dev, struct
>>> sk_buff_head *q)
>>> int count = 0;
>>>
>>> spin_lock_irqsave (&q->lock, flags);
>>> + set_cpu_bit(URB_UNLINKING, dev->cpuflags);
>>> skb_queue_walk_safe(q, skb, skbnext) {
>>> struct skb_data *entry;
>>> struct urb *urb;
>>> @@ -590,15 +595,6 @@ static int unlink_urbs (struct usbnet *dev,
>>> struct sk_buff_head *q)
>>> entry = (struct skb_data *) skb->cb;
>>> urb = entry->urb;
>>>
>>> - /*
>>> - * Get reference count of the URB to avoid it to be
>>> - * freed during usb_unlink_urb, which may trigger
>>> - * use-after-free problem inside usb_unlink_urb since
>>> - * usb_unlink_urb is always racing with .complete
>>> - * handler(include defer_bh).
>>> - */
>>> - usb_get_urb(urb);
>>> - spin_unlock_irqrestore(&q->lock, flags);
>>> // during some PM-driven resume scenarios,
>>> // these (async) unlinks complete immediately
>>> retval = usb_unlink_urb (urb);
>>> @@ -606,9 +602,8 @@ static int unlink_urbs (struct usbnet *dev, struct
>>> sk_buff_head *q)
>>> netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
>>> else
>>> count++;
>>> - usb_put_urb(urb);
>>> - spin_lock_irqsave(&q->lock, flags);
>>> }
>>> + clear_cpu_bit(URB_UNLINKING, dev->cpuflags);
>>> spin_unlock_irqrestore (&q->lock, flags);
>>> return count;
>>> }
>>> @@ -1283,6 +1278,7 @@ void usbnet_disconnect (struct usb_interface *intf)
>>> usb_kill_urb(dev->interrupt);
>>> usb_free_urb(dev->interrupt);
>>>
>>> + free_percpu(dev->cpuflags);
>>> free_netdev(net);
>>> usb_put_dev (xdev);
>>> }
>>> @@ -1353,6 +1349,13 @@ usbnet_probe (struct usb_interface *udev, const
>>> struct usb_device_id *prod)
>>> SET_NETDEV_DEV(net, &udev->dev);
>>>
>>> dev = netdev_priv(net);
>>> +
>>> + dev->cpuflags = alloc_percpu(unsigned long);
>>> + if (!dev->cpuflags) {
>>> + status = -ENOMEM;
>>> + goto out1;
>>> + }
>>> +
>>> dev->udev = xdev;
>>> dev->intf = udev;
>>> dev->driver_info = info;
>>> @@ -1396,7 +1399,7 @@ usbnet_probe (struct usb_interface *udev, const
>>> struct usb_device_id *prod)
>>> if (info->bind) {
>>> status = info->bind (dev, udev);
>>> if (status < 0)
>>> - goto out1;
>>> + goto out2;
>>>
>>> // heuristic: "usb%d" for links we know are two-host,
>>> // else "eth%d" when there's reasonable doubt. userspace
>>> @@ -1465,6 +1468,8 @@ usbnet_probe (struct usb_interface *udev, const
>>> struct usb_device_id *prod)
>>> out3:
>>> if (info->unbind)
>>> info->unbind (dev, udev);
>>> +out2:
>>> + free_percpu(dev->cpuflags);
>>> out1:
>>> free_netdev(net);
>>> out:
>>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
>>> index 605b0aa..2dc46f5 100644
>>> --- a/include/linux/usb/usbnet.h
>>> +++ b/include/linux/usb/usbnet.h
>>> @@ -69,8 +69,28 @@ struct usbnet {
>>> # define EVENT_DEV_WAKING 6
>>> # define EVENT_DEV_ASLEEP 7
>>> # define EVENT_DEV_OPEN 8
>>> + unsigned long __percpu *cpuflags;
>>> +# define URB_UNLINKING 0
>>
>> Is it possible using a simple bool variable to track whether q->lock
>> is hold by unlink_urb() ? If yes, it can avoid introducing following
>> new codes into current code stack.
>
> It should be defined as percpu variable. The URB complete handler
> may be triggered inside unlink path, or it can be triggered in hardirq path
> from other CPUs at the same time.
>
Then the potential issue is, in defer_bh(), __skb_unlink() may be
called without holding the lock. This can cause current issue too,
because __skb_unlink() may move 'skb->next' out of txq/rxq, and even
free it.
>>
>>> };
>>>
>>> +static inline void set_cpu_bit(int nr, unsigned long __percpu *addr)
>>> +{
>>> + unsigned long *fl = __this_cpu_ptr(addr);
>>> + set_bit(nr, fl);
>>> +}
>>> +
>>> +static inline void clear_cpu_bit(int nr, unsigned long __percpu *addr)
>>> +{
>>> + unsigned long *fl = __this_cpu_ptr(addr);
>>> + clear_bit(nr, fl);
>>> +}
>>> +
>>> +static inline int test_cpu_bit(int nr, unsigned long __percpu *addr)
>>> +{
>>> + unsigned long *fl = __this_cpu_ptr(addr);
>>> + return test_bit(nr, fl);
>>> +}
>>> +
>>> static inline struct usb_driver *driver_of(struct usb_interface *intf)
>>> {
>>> return to_usb_driver(intf->dev.driver);
>>>
>>>
>>> Thanks,
>>> --
>>> Ming Lei
>
>
>
> --
> Ming Lei
^ permalink raw reply
* tc: problem with nested classes
From: Ireneusz Szcześniak @ 2012-04-22 12:52 UTC (permalink / raw)
To: netdev
I have a problem understanding nested classes in tc (traffic control).
To introduce the problem, I briefly describe how I think the tc works.
There are qdiscs, classes and filters. A qdisc can have classes,
and filters configured for this qdisc classify packets. When a packet
is enqueue to a qdisc, filters decide which class the packet belongs
to, i.e. filters classify packets. Each class has a qdisc, by default
the pfifo class, and so classifying implies enqueing a packet in the
queue of class the packet was classified to.
Therefore I would claim that a class cannot have another class as a
child, because the child should be a qdisc. However, Example 1 shows
that I'm wrong.
Example 1
---------
tc qdisc add dev eth0 root handle 1:0 htb
tc class add dev eth0 parent 1:0 classid 1:1 htb rate 10mbit
tc class add dev eth0 parent 1:1 classid 1:2 htb rate 1mbit
In this example class 1:1 is a parent of both classes 1:2 and 1:3.
QUESTION 1: Does class 1:1 have a queue?
Example 2 implements the same functionality as Example 1, I think.
Example 2
---------
tc qdisc add dev eth0 root handle 1:0 htb
tc class add dev eth0 parent 1:0 classid 1:1 htb rate 10mbit
tc qdisc add dev eth0 parent 1:1 handle 2:0 htb
tc class add dev eth0 parent 2:0 classid 2:1 htb rate 1mbit
QUESTION 2: Is there a difference between Example 1 and Example 2?
QUESTION 3: What is "-d" for in "tc -s -d qdisc"
QUESTION 4: How can I view the statistics for default queues of
classes, those pfifo queues? "tc -s qdisc" doesn't show them.
QUESTION 5: Why in filters there is used "flowid" instead of "parent"?
I would appreciate it if someone could answer my questions.
--
Ireneusz (Irek) Szczesniak
http://www.irkos.org
^ permalink raw reply
* Re: use-after-free in usbnet
From: Ming Lei @ 2012-04-22 12:05 UTC (permalink / raw)
To: Huajun Li
Cc: Oliver Neukum, Alan Stern, Dave Jones, netdev, linux-usb,
Fedora Kernel Team
In-Reply-To: <CA+v9cxaqL1V3eyv=dhKbqbnXk6wm1qm93DNFbrbwbd1b1X+3rw@mail.gmail.com>
On Sun, Apr 22, 2012 at 10:19 AM, Huajun Li <huajun.li.lee@gmail.com> wrote:
> On Sat, Apr 21, 2012 at 3:45 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>> Hi Huajun,
>>
>> On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>>>> Just skip trying this per your following email's comments.
>>>
>>> I will prepare a new patch later, if you'd like to try it.
>>
>> The below patch reverts the below commits:
>>
>> 0956a8c20b23d429e79ff86d4325583fc06f9eb4
>> (usbnet: increase URB reference count before usb_unlink_urb)
>>
>> 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d
>> (net/usbnet: avoid recursive locking in usbnet_stop())
>>
>> and keep holding tx/rx queue lock during unlinking, but avoid
>> to acquire the same queue lock inside complete handler triggered by
>> usb_unlink_urb.
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index db99536..effb34e 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -291,9 +291,11 @@ static void defer_bh(struct usbnet *dev, struct
>> sk_buff *skb, struct sk_buff_hea
>> {
>> unsigned long flags;
>>
>> - spin_lock_irqsave(&list->lock, flags);
>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>> + spin_lock_irqsave(&list->lock, flags);
>> __skb_unlink(skb, list);
>> - spin_unlock(&list->lock);
>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>> + spin_unlock(&list->lock);
>> spin_lock(&dev->done.lock);
>> __skb_queue_tail(&dev->done, skb);
>> if (dev->done.qlen == 1)
>
>
> Then 'flags' may not be initialized, and this will cause problem while
> calling spin_unlock_irqrestore(&dev->done.lock, flags), right?
The flag is a percpu variable, so it can't change during the
above code piece.
>
>
>> @@ -345,7 +347,8 @@ static int rx_submit (struct usbnet *dev, struct
>> urb *urb, gfp_t flags)
>> usb_fill_bulk_urb (urb, dev->udev, dev->in,
>> skb->data, size, rx_complete, skb);
>>
>> - spin_lock_irqsave (&dev->rxq.lock, lockflags);
>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>> + spin_lock_irqsave (&dev->rxq.lock, lockflags);
>>
>> if (netif_running (dev->net) &&
>> netif_device_present (dev->net) &&
>> @@ -377,7 +380,8 @@ static int rx_submit (struct usbnet *dev, struct
>> urb *urb, gfp_t flags)
>> netif_dbg(dev, ifdown, dev->net, "rx: stopped\n");
>> retval = -ENOLINK;
>> }
>> - spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
>> + if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>> + spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
>> if (retval) {
>> dev_kfree_skb_any (skb);
>> usb_free_urb (urb);
>> @@ -582,6 +586,7 @@ static int unlink_urbs (struct usbnet *dev, struct
>> sk_buff_head *q)
>> int count = 0;
>>
>> spin_lock_irqsave (&q->lock, flags);
>> + set_cpu_bit(URB_UNLINKING, dev->cpuflags);
>> skb_queue_walk_safe(q, skb, skbnext) {
>> struct skb_data *entry;
>> struct urb *urb;
>> @@ -590,15 +595,6 @@ static int unlink_urbs (struct usbnet *dev,
>> struct sk_buff_head *q)
>> entry = (struct skb_data *) skb->cb;
>> urb = entry->urb;
>>
>> - /*
>> - * Get reference count of the URB to avoid it to be
>> - * freed during usb_unlink_urb, which may trigger
>> - * use-after-free problem inside usb_unlink_urb since
>> - * usb_unlink_urb is always racing with .complete
>> - * handler(include defer_bh).
>> - */
>> - usb_get_urb(urb);
>> - spin_unlock_irqrestore(&q->lock, flags);
>> // during some PM-driven resume scenarios,
>> // these (async) unlinks complete immediately
>> retval = usb_unlink_urb (urb);
>> @@ -606,9 +602,8 @@ static int unlink_urbs (struct usbnet *dev, struct
>> sk_buff_head *q)
>> netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
>> else
>> count++;
>> - usb_put_urb(urb);
>> - spin_lock_irqsave(&q->lock, flags);
>> }
>> + clear_cpu_bit(URB_UNLINKING, dev->cpuflags);
>> spin_unlock_irqrestore (&q->lock, flags);
>> return count;
>> }
>> @@ -1283,6 +1278,7 @@ void usbnet_disconnect (struct usb_interface *intf)
>> usb_kill_urb(dev->interrupt);
>> usb_free_urb(dev->interrupt);
>>
>> + free_percpu(dev->cpuflags);
>> free_netdev(net);
>> usb_put_dev (xdev);
>> }
>> @@ -1353,6 +1349,13 @@ usbnet_probe (struct usb_interface *udev, const
>> struct usb_device_id *prod)
>> SET_NETDEV_DEV(net, &udev->dev);
>>
>> dev = netdev_priv(net);
>> +
>> + dev->cpuflags = alloc_percpu(unsigned long);
>> + if (!dev->cpuflags) {
>> + status = -ENOMEM;
>> + goto out1;
>> + }
>> +
>> dev->udev = xdev;
>> dev->intf = udev;
>> dev->driver_info = info;
>> @@ -1396,7 +1399,7 @@ usbnet_probe (struct usb_interface *udev, const
>> struct usb_device_id *prod)
>> if (info->bind) {
>> status = info->bind (dev, udev);
>> if (status < 0)
>> - goto out1;
>> + goto out2;
>>
>> // heuristic: "usb%d" for links we know are two-host,
>> // else "eth%d" when there's reasonable doubt. userspace
>> @@ -1465,6 +1468,8 @@ usbnet_probe (struct usb_interface *udev, const
>> struct usb_device_id *prod)
>> out3:
>> if (info->unbind)
>> info->unbind (dev, udev);
>> +out2:
>> + free_percpu(dev->cpuflags);
>> out1:
>> free_netdev(net);
>> out:
>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
>> index 605b0aa..2dc46f5 100644
>> --- a/include/linux/usb/usbnet.h
>> +++ b/include/linux/usb/usbnet.h
>> @@ -69,8 +69,28 @@ struct usbnet {
>> # define EVENT_DEV_WAKING 6
>> # define EVENT_DEV_ASLEEP 7
>> # define EVENT_DEV_OPEN 8
>> + unsigned long __percpu *cpuflags;
>> +# define URB_UNLINKING 0
>
> Is it possible using a simple bool variable to track whether q->lock
> is hold by unlink_urb() ? If yes, it can avoid introducing following
> new codes into current code stack.
It should be defined as percpu variable. The URB complete handler
may be triggered inside unlink path, or it can be triggered in hardirq path
from other CPUs at the same time.
>
>> };
>>
>> +static inline void set_cpu_bit(int nr, unsigned long __percpu *addr)
>> +{
>> + unsigned long *fl = __this_cpu_ptr(addr);
>> + set_bit(nr, fl);
>> +}
>> +
>> +static inline void clear_cpu_bit(int nr, unsigned long __percpu *addr)
>> +{
>> + unsigned long *fl = __this_cpu_ptr(addr);
>> + clear_bit(nr, fl);
>> +}
>> +
>> +static inline int test_cpu_bit(int nr, unsigned long __percpu *addr)
>> +{
>> + unsigned long *fl = __this_cpu_ptr(addr);
>> + return test_bit(nr, fl);
>> +}
>> +
>> static inline struct usb_driver *driver_of(struct usb_interface *intf)
>> {
>> return to_usb_driver(intf->dev.driver);
>>
>>
>> Thanks,
>> --
>> Ming Lei
--
Ming Lei
^ 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