* [flame^Wreview] net: netprio_cgroup: rework update socket logic
From: Al Viro @ 2012-08-13 1:53 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Neil Horman, John Fastabend, linux-kernel
Ladies and gentlemen, who the devil had reviewed that little gem?
commit 406a3c638ce8b17d9704052c07955490f732c2b8
Author: John Fastabend <john.r.fastabend@intel.com>
Date: Fri Jul 20 10:39:25 2012 +0000
is a bleeding bogosity that doesn't pass even the most cursory
inspection. It iterates through descriptor tables of a bunch
of processes, doing this:
file = fcheck_files(files, fd);
if (!file)
continue;
path = d_path(&file->f_path, tmp, PAGE_SIZE);
rv = sscanf(path, "socket:[%lu]", &s);
if (rv <= 0)
continue;
sock = sock_from_file(file, &err);
if (!err)
sock_update_netprioidx(sock->sk, p);
Note the charming use of sscanf() for pattern-matching. 's' (inode
number of socket) is completely unused afterwards; what happens here
is a very badly written attempt to skip non-sockets. Why, will
sock_from_file() blow up on non-sockets? And isn't there some less
obnoxious way to check that the file is a sockfs one? Let's see:
struct socket *sock_from_file(struct file *file, int *err)
{
if (file->f_op == &socket_file_ops)
return file->private_data; /* set in sock_map_fd */
*err = -ENOTSOCK;
return NULL;
}
... and the first line is exactly that - a check that we are on sockfs.
_Far_ less expensive one, at that, so it's not even that we are avoiding
a costly test. In other words, all masturbation with d_path() is absolutely
pointless.
Furthermore, it's racy; had been even more so before the delayed fput series
went in, but even now it's not safe. fcheck_files() does *NOT* guarantee
that file is not getting closed right now. rcu_read_lock() prevents only
freeing and potential reuse of struct file we'd got; it might go all the
way through final fput() just as we look at it. So file->f_path is not
protected by anything. Worse yet, neither is struct socket itself - we
might be going through sock_release() at the same time, so sock->sk might
very well be NULL, leaving us a oops even after we dump d_path() idiocy.
To make it even funnier, there's such thing as SCM_RIGHTS datagrams and
descriptor passing. In other words, it's *not* going to catch all sockets
that would be caught by the earlier variant.
What the hell it is about cgroups that turns otherwise sane people into
gibbering idiots? Other than the Old Ones' apparent involvement in
the mental processes that had lead to the entire concept, that is...
Let's take a closer look at the entire net/core/netprio_cgroup.c, shall we?
Right at the beginning of that Fine Piece of Software we find this:
static atomic_t max_prioidx = ATOMIC_INIT(0);
Why is it atomic? We have all of four accesses to it; one writer and three
readers. The writer and one of the readers are under the same spinlock;
the rest of readers are under rtnl_lock(). Moreover, the sole writer is
in get_prioidx(), which is called in one place and is immediately followed
by grabbing rtnl_lock(). So shifting it down there would've put *all*
accesses of that sucker under the same mutex...
Next to that one we have
static DEFINE_SPINLOCK(prioidx_map_lock);
What is that one for? Two places touching that sucker; get_prioidx() and
put_prioidx(). spin_lock_irqsave()/spin_unlock_irqrestore() in both.
Why irqsave? Somebody calling that from interrupt context? Looks odd...
OK, there's not a lot of callers - cgrp_create(), cgrp_destroy(). And
neither looks like something that could be called from an interrupt.
The former has GFP_KERNEL allocation right in the beginning. Oh, lookie -
both do rtnl_lock(). So not only is all this wanking with irqs pure
cargo-culting, the whole spinlock is pointless; we can simply shift all
this stuff under rtnl_lock().
After get_prioidx()/put_priodix() we come to the following gem:
for (i = 0;
old_priomap && (i < old_priomap->priomap_len);
i++)
new_priomap->priomap[i] = old_priomap->priomap[i];
Why are we checking old_priomap on every step? Sure, gcc will take
that out of loop, but why obfuscate the damn thing?
if (old_priomap)
memcpy(new_priomap->priomap, old_priomap,
sizeof(u32) * old_priomap->priomap_len);
would be far more idiomatic... Anyway, we are extending the damn array,
copying contents of the old one to replacement. Understandable. Where
do we modify the contents of that array, anyway? Aha:
rtnl_lock();
for_each_netdev(&init_net, dev) {
map = rtnl_dereference(dev->priomap);
if (map && cs->prioidx < map->priomap_len)
map->priomap[cs->prioidx] = 0;
}
rtnl_unlock();
and
rcu_read_lock();
map = rcu_dereference(dev->priomap);
if (map)
map->priomap[prioidx] = priority;
rcu_read_unlock();
The first one is serialized with the reallocate-and-copy by rtnl_lock().
The second one, though, is immediately suspicious - rcu_read_lock() in
updater. It's in write_priomap(), which is ->write_string() in some object
deep in the bloated bowels of cgroup. Only one caller: cgroup_write_string().
No locks grabbed. Called from cgroup_file_write(), again without any locks.
Which is ->write() of file_operations. And that is callable without any
locks whatsoever. OK, so we definitely have a race here.
Incidentally, we'd dropped rtnl_lock() just before that point. IOW, it's
trivially fixed by moving that thing into write_update_netdev_table()...
BTW, speaking of highly non-idiomatic code: strstr(devname, " "). Why
not introduce strcasestr(), if we are going for maximal obfuscation here?
Sigh...
^ permalink raw reply
* Re: [flame^Wreview] net: netprio_cgroup: rework update socket logic
From: Al Viro @ 2012-08-13 2:30 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Neil Horman, John Fastabend, linux-kernel
In-Reply-To: <20120813015348.GZ23464@ZenIV.linux.org.uk>
On Mon, Aug 13, 2012 at 02:53:48AM +0100, Al Viro wrote:
> if (old_priomap)
> memcpy(new_priomap->priomap, old_priomap,
^^^^^^^^^^^
old_priomap->priomap,
that is.
^ permalink raw reply
* Re: [PATCH 1/2] net: move and rename netif_notify_peers()
From: Cong Wang @ 2012-08-13 2:46 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, David S. Miller, Ian Campbell
In-Reply-To: <1344625276.2701.10.camel@bwh-desktop.uk.solarflarecom.com>
On Fri, 2012-08-10 at 20:01 +0100, Ben Hutchings wrote:
> On Fri, 2012-08-10 at 16:14 +0800, Cong Wang wrote:
> > I believe net/core/dev.c is a better place for netif_notify_peers(),
> > because other net event notify functions also stay in this file.
> >
> > And rename it to netdev_notify_peers().
> [...]
>
> Is there a convention for using the 'netdev' vs 'netif' prefixes?
> If not, I don't see the point in renaming just this one function.
>
The reason why I rename it is there are more functions named netdev_*
than netif_* in net/core/dev.c. Also given that netdev_bonding_change()
has netdev_ prefix too.
I don't have strong opinions on this.
Thanks.
^ permalink raw reply
* Re: [Question]About KVM network zero-copy feature!
From: Robert Vineyard @ 2012-08-13 3:45 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xiaohui.xin, kvm, netdev, Peter Huang(Peng), virtualization, avi,
Stephen Hemminger
In-Reply-To: <20120812093730.GD1421@redhat.com>
On 08/12/2012 05:37 AM, Michael S. Tsirkin wrote:
> AFAIK Xin left Intel and is not working on it.
> Contributions are welcome.
That's too bad... do you know of anyone else (at Intel or otherwise) who
might be familiar enough with the existing codebase to get me started?
From the code I've looked at, it appears that among other things
they're using the splice(2) / vmsplice(2) system calls to effect
"copying" without actually copying data. If I understand the semantics
correctly, these calls are basically shuffling pointers around to avoid
unnecessary memcpy(3) / mmap(2) calls. I've even seen a "zero-copy"
version of sendfile(2) that essentially wraps it around a call to splice(2).
I may be able to hack something together based on the current zero-copy
TX implementation, but as I'm still wrapping my head around several of
the concepts I just described, it may be awhile before I can produce
anything useful. I have quite a bit of experience developing for Linux
in C, but this would be my first attempt at writing kernel/device-driver
code.
>> The Release Notes for RHEL 6.2 (originally published on 12/06/2011)
>> also specifically mention macvtap/vhost zero-copy capabilities as
>> being included as a Technology Preview:
>>
>> http://docs.redhat.com/docs/en-US/Red_Hat_Enterprise_Linux/6/html/6.2_Release_Notes/virtualization.html
>
> I think this means TX.
It does. I meant to clarify that point in my original email... yes, only
TX zero-copy is currently implemented, and it is still marked as
"experimental". Outside of the custom solutions like PF_RING that I
mentioned, I don't know that I've seen zero-copy for RX.
-- Robert Vineyard
^ permalink raw reply
* Re: [flame^Wreview] net: netprio_cgroup: rework update socket logic
From: John Fastabend @ 2012-08-13 5:55 UTC (permalink / raw)
To: Al Viro; +Cc: netdev, David Miller, Neil Horman, linux-kernel
In-Reply-To: <20120813015348.GZ23464@ZenIV.linux.org.uk>
On 8/12/2012 6:53 PM, Al Viro wrote:
> Ladies and gentlemen, who the devil had reviewed that little gem?
>
> commit 406a3c638ce8b17d9704052c07955490f732c2b8
> Author: John Fastabend <john.r.fastabend@intel.com>
> Date: Fri Jul 20 10:39:25 2012 +0000
>
> is a bleeding bogosity that doesn't pass even the most cursory
> inspection. It iterates through descriptor tables of a bunch
> of processes, doing this:
> file = fcheck_files(files, fd);
> if (!file)
> continue;
>
> path = d_path(&file->f_path, tmp, PAGE_SIZE);
> rv = sscanf(path, "socket:[%lu]", &s);
> if (rv <= 0)
> continue;
>
> sock = sock_from_file(file, &err);
> if (!err)
> sock_update_netprioidx(sock->sk, p);
> Note the charming use of sscanf() for pattern-matching. 's' (inode
> number of socket) is completely unused afterwards; what happens here
> is a very badly written attempt to skip non-sockets. Why, will
> sock_from_file() blow up on non-sockets? And isn't there some less
> obnoxious way to check that the file is a sockfs one? Let's see:
> struct socket *sock_from_file(struct file *file, int *err)
> {
> if (file->f_op == &socket_file_ops)
> return file->private_data; /* set in sock_map_fd */
>
> *err = -ENOTSOCK;
> return NULL;
> }
> ... and the first line is exactly that - a check that we are on sockfs.
> _Far_ less expensive one, at that, so it's not even that we are avoiding
> a costly test. In other words, all masturbation with d_path() is absolutely
> pointless.
>
> Furthermore, it's racy; had been even more so before the delayed fput series
> went in, but even now it's not safe. fcheck_files() does *NOT* guarantee
> that file is not getting closed right now. rcu_read_lock() prevents only
> freeing and potential reuse of struct file we'd got; it might go all the
> way through final fput() just as we look at it. So file->f_path is not
> protected by anything. Worse yet, neither is struct socket itself - we
> might be going through sock_release() at the same time, so sock->sk might
> very well be NULL, leaving us a oops even after we dump d_path() idiocy.
>
> To make it even funnier, there's such thing as SCM_RIGHTS datagrams and
> descriptor passing. In other words, it's *not* going to catch all sockets
> that would be caught by the earlier variant.
>
OK clearly I screwed it up thanks for reviewing Al. How about this.
fdt = files_fdtable(files);
for (fd = 0; fd < fdt->max_fds; fd++) {
struct socket *sock;
int err = 0;
sock = sockfd_lookup(fd, &err);
if (!sock) {
lock_sock(sock->sk);
sock_update_netprioidx(sock->sk, p);
release_sock(sock->sk);
sockfd_put(sock);
}
}
sockfd_lookup will call fget() and also test file->f_op. testing this
now.
^ permalink raw reply
* Re: [Question]About KVM network zero-copy feature!
From: Robert Vineyard @ 2012-08-13 5:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xiaohui.xin, kvm, netdev, Peter Huang(Peng), virtualization, avi,
Stephen Hemminger
In-Reply-To: <50287857.2060305@tuffmail.com>
On 08/12/2012 11:45 PM, Robert Vineyard wrote:
> On 08/12/2012 05:37 AM, Michael S. Tsirkin wrote:
>> Contributions are welcome.
>
> That's too bad... do you know of anyone else (at Intel or otherwise) who
> might be familiar enough with the existing codebase to get me started?
I suppose I should have done my homework... *ahem* time for me to finish
RTFM on git...
I'll come back after digging through more kernel code :-)
-- Robert
^ permalink raw reply
* [GIT] Networking
From: David Miller @ 2012-08-13 6:06 UTC (permalink / raw)
To: torvalds; +Cc: akpm, netdev, linux-kernel
Most importantly this should cure the ipv4-mapped ipv6 socket TCP
crashes some people were seeing, otherwise:
1) Fix e1000e autonegotiation handling regression, from Tushar
Dave.
2) Fix TX data corruption race on e1000e down, also from Tushar
Dave.
3) Fix bfin_sir IRDA driver build, from Sonic Zhang.
4) AF_PACKET mmap() tests a flag in the TX ring shared between
userspace and the kernel for an internal consistency check. It
really shouldn't do this to validate the kernel's own behavior
because the user can corrupt it to be any value at all. From
Daniel Borkmann.
5) Fix TCP metrics leak on netns dismantle, from Eric Dumazet.
6) Orphan the anonymous TCP socket from the SKB in
ip_send_unicast_reply() so that the rest of the stack
needn't see it. Otherwise we get selinux problems of
all sorts, from Eric Dumazet.
This is the best way to fix this since the socket is just a
place holder for sending packets in a context where we have
no real socket at all.
7) Fix TUN detach crashes, from Stanislav Kinsbursky.
8) dev_set_alias() leaks memory on krealloc() failure, from Alexey
Khoroshilov.
9) FIB trie must use call_rcu() not call_rcu_bh(), because this code
is not universally invoked from software interrupts. From Eric
Dumazet.
10) PPTP looks up ipv4 routes with the wrong network namespace, fix from
Gao Feng.
Please pull, thanks a lot!
The following changes since commit f4ba394c1b02e7fc2179fda8d3941a5b3b65efb6:
Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2012-08-08 20:06:43 +0300)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git master
for you to fetch changes up to f57b07c0c7ca9e4dde36acfabdf474ee3c478e6d:
bnx2x: Fix compiler warnings (2012-08-12 13:42:18 -0700)
----------------------------------------------------------------
Alexander Duyck (1):
igb: Fix register defines for all non-82575 hardware
Alexey Khoroshilov (1):
net/core: Fix potential memory leak in dev_set_alias()
Arnd Bergmann (1):
net/stmmac: mark probe function as __devinit
Daniel Drake (1):
cfg80211: process pending events when unregistering net device
David S. Miller (3):
Merge branch 'master' of git://git.kernel.org/.../ppwaskie/net
Merge branch 'fixes-for-3.6' of git://gitorious.org/linux-can/linux-can
Merge branch 'for-davem' of git://git.kernel.org/.../linville/wireless
Denis Efremov (1):
macvtap: rcu_dereference outside read-lock section
Emil Tantilov (3):
igb: fix panic while dumping packets on Tx hang with IOMMU
e1000e: fix panic while dumping packets on Tx hang with IOMMU
ixgbe: add missing braces
Eric Dumazet (7):
net: fib: fix incorrect call_rcu_bh()
net: force dst_default_metrics to const section
tcp: must free metrics at net dismantle
ipv4: tcp: unicast_sock should not land outside of TCP stack
net: tcp: ipv6_mapped needs sk_rx_dst_set method
ipv4: fix ip_send_skb()
codel: refine one condition to avoid a nul rec_inv_sqrt
Gao feng (1):
pptp: lookup route with the proper net namespace
Jesper Juhl (2):
batman-adv: Fix mem leak in the batadv_tt_local_event() function
cdc-phonet: Don't leak in usbpn_open
Johannes Berg (1):
iwlwifi: disable greenfield transmissions as a workaround
John W. Linville (1):
Merge branch 'master' of git://git.kernel.org/.../linville/wireless into for-davem
Joren Van Onder (1):
bnx2x: Fix compiler warnings
Oliver Hartkopp (1):
canfd: remove redundant CAN FD flag
Paolo Valente (1):
sched: add missing group change to qfq_change_class
Sonic Zhang (1):
drivers: net: irda: bfin_sir: fix compile error
Stanislav Kinsbursky (1):
tun: don't zeroize sock->file on detach
Stanislaw Gruszka (1):
rt61pci: fix NULL pointer dereference in config_lna_gain
Stefan Assmann (1):
igb: add delay to allow igb loopback test to succeed on 8086:10c9
Tushar Dave (2):
e1000e: NIC goes up and immediately goes down
e1000e: 82571 Tx Data Corruption during Tx hang recovery
Ying Xue (1):
af_packet: Quiet sparse noise about using plain integer as NULL pointer
Yuval Mintz (2):
bnx2x: fix unload previous driver flow when flr-capable
bnx2x: Fix recovery flow cleanup during probe
danborkmann@iogearbox.net (1):
af_packet: remove BUG statement in tpacket_destruct_skb
stigge@antcom.de (1):
lpc_eth: remove obsolete ifdefs
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 +-
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 72 ++++++++++++++++------------------------
drivers/net/ethernet/intel/e1000e/82571.c | 10 +++---
drivers/net/ethernet/intel/e1000e/netdev.c | 36 ++++++++++++++------
drivers/net/ethernet/intel/igb/e1000_regs.h | 8 +++--
drivers/net/ethernet/intel/igb/igb_ethtool.c | 3 ++
drivers/net/ethernet/intel/igb/igb_main.c | 19 +++++------
drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c | 3 +-
drivers/net/ethernet/nxp/lpc_eth.c | 13 --------
drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 2 +-
drivers/net/irda/bfin_sir.c | 8 ++---
drivers/net/macvtap.c | 3 +-
drivers/net/ppp/pptp.c | 4 +--
drivers/net/tun.c | 1 -
drivers/net/usb/cdc-phonet.c | 1 +
drivers/net/wireless/iwlwifi/dvm/rs.c | 13 +++++---
drivers/net/wireless/rt2x00/rt61pci.c | 3 +-
include/linux/can.h | 25 +++++++-------
include/net/codel.h | 8 +++--
include/net/dst.h | 2 +-
include/net/ip.h | 2 +-
include/net/tcp.h | 1 +
net/batman-adv/translation-table.c | 1 +
net/core/dev.c | 7 ++--
net/core/dst.c | 10 +++++-
net/ipv4/fib_trie.c | 2 +-
net/ipv4/ip_output.c | 6 ++--
net/ipv4/tcp_ipv4.c | 3 +-
net/ipv4/tcp_metrics.c | 12 +++++++
net/ipv4/udp.c | 2 +-
net/ipv6/tcp_ipv6.c | 1 +
net/packet/af_packet.c | 3 +-
net/sched/sch_qfq.c | 95 ++++++++++++++++++++++++++++++++++++++---------------
net/wireless/core.c | 5 +++
net/wireless/core.h | 1 +
net/wireless/util.c | 2 +-
36 files changed, 233 insertions(+), 156 deletions(-)
^ permalink raw reply
* [PATCH] ipv4: Cache local output routes
From: Yan, Zheng @ 2012-08-13 6:09 UTC (permalink / raw)
To: netdev@vger.kernel.org, davem@davemloft.net; +Cc: Shi, Alex
Commit caacf05e5ad1abf causes big drop of UDP loop back performance.
The cause of the regression is that we do not cache the local output
routes. Each time we send a datagram from unconnected UDP socket,
the kernel allocates a dst_entry and adds it to the rt_uncached_list.
It creates lock contention on the rt_uncached_lock.
Reported-by: Alex Shi <alex.shi@intel.com>
Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index e4ba974..fd9ecb5 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2028,7 +2028,6 @@ struct rtable *__ip_route_output_key(struct net *net, struct flowi4 *fl4)
}
dev_out = net->loopback_dev;
fl4->flowi4_oif = dev_out->ifindex;
- res.fi = NULL;
flags |= RTCF_LOCAL;
goto make_route;
}
^ permalink raw reply related
* Re: [flame^Wreview] net: netprio_cgroup: rework update socket logic
From: John Fastabend @ 2012-08-13 6:23 UTC (permalink / raw)
To: Al Viro; +Cc: netdev, David Miller, Neil Horman, linux-kernel
In-Reply-To: <502896C5.7080303@intel.com>
> OK clearly I screwed it up thanks for reviewing Al. How about this.
>
> fdt = files_fdtable(files);
> for (fd = 0; fd < fdt->max_fds; fd++) {
> struct socket *sock;
> int err = 0;
>
> sock = sockfd_lookup(fd, &err);
> if (!sock) {
typo ^^^^^^
if (sock) {
to be honest I can't see why I didn't use sockfd_lookup in the first
place...
^ permalink raw reply
* [PATCH] XFRM: remove redundant parameter "int dir" in struct xfrm_mgr.acquire
From: Fan Du @ 2012-08-13 6:25 UTC (permalink / raw)
To: davem; +Cc: netdev
Sematically speaking, xfrm_mgr.acquire is called when kernel intends to ask
user space IKE daemon to negotiate SAs with peers. IOW the direction will
*always* be XFRM_POLICY_OUT, so remove int dir for clarity.
Signed-off-by: Fan Du <fan.du@windriver.com>
---
include/net/xfrm.h | 2 +-
net/key/af_key.c | 4 ++--
net/xfrm/xfrm_state.c | 2 +-
net/xfrm/xfrm_user.c | 4 ++--
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 62b619e..5e1662d 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -571,7 +571,7 @@ struct xfrm_mgr {
struct list_head list;
char *id;
int (*notify)(struct xfrm_state *x, const struct km_event *c);
- int (*acquire)(struct xfrm_state *x, struct xfrm_tmpl *, struct xfrm_policy *xp, int dir);
+ int (*acquire)(struct xfrm_state *x, struct xfrm_tmpl *, struct xfrm_policy *xp);
struct xfrm_policy *(*compile_policy)(struct sock *sk, int opt, u8 *data, int len, int *dir);
int (*new_mapping)(struct xfrm_state *x, xfrm_address_t *ipaddr, __be16 sport);
int (*notify_policy)(struct xfrm_policy *x, int dir, const struct km_event *c);
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 34e4185..ec7d161 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -3024,7 +3024,7 @@ static u32 get_acqseq(void)
return res;
}
-static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct xfrm_policy *xp, int dir)
+static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct xfrm_policy *xp)
{
struct sk_buff *skb;
struct sadb_msg *hdr;
@@ -3105,7 +3105,7 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
pol->sadb_x_policy_len = sizeof(struct sadb_x_policy)/sizeof(uint64_t);
pol->sadb_x_policy_exttype = SADB_X_EXT_POLICY;
pol->sadb_x_policy_type = IPSEC_POLICY_IPSEC;
- pol->sadb_x_policy_dir = dir+1;
+ pol->sadb_x_policy_dir = XFRM_POLICY_OUT + 1;
pol->sadb_x_policy_id = xp->index;
/* Set sadb_comb's. */
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 87cd0e4..7856c33 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1700,7 +1700,7 @@ int km_query(struct xfrm_state *x, struct xfrm_tmpl *t, struct xfrm_policy *pol)
read_lock(&xfrm_km_lock);
list_for_each_entry(km, &xfrm_km_list, list) {
- acqret = km->acquire(x, t, pol, XFRM_POLICY_OUT);
+ acqret = km->acquire(x, t, pol);
if (!acqret)
err = acqret;
}
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index e75d8e4..844e661 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2605,7 +2605,7 @@ static int build_acquire(struct sk_buff *skb, struct xfrm_state *x,
}
static int xfrm_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *xt,
- struct xfrm_policy *xp, int dir)
+ struct xfrm_policy *xp)
{
struct net *net = xs_net(x);
struct sk_buff *skb;
@@ -2614,7 +2614,7 @@ static int xfrm_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *xt,
if (skb == NULL)
return -ENOMEM;
- if (build_acquire(skb, x, xt, xp, dir) < 0)
+ if (build_acquire(skb, x, xt, xp, XFRM_POLICY_OUT) < 0)
BUG();
return nlmsg_multicast(net->xfrm.nlsk, skb, 0, XFRMNLGRP_ACQUIRE, GFP_ATOMIC);
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] XFRM: remove redundant parameter "int dir" in struct xfrm_mgr.acquire
From: Steffen Klassert @ 2012-08-13 7:08 UTC (permalink / raw)
To: Fan Du; +Cc: davem, netdev
In-Reply-To: <1344839157-25797-1-git-send-email-fan.du@windriver.com>
On Mon, Aug 13, 2012 at 02:25:57PM +0800, Fan Du wrote:
>
> static int xfrm_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *xt,
> - struct xfrm_policy *xp, int dir)
> + struct xfrm_policy *xp)
> {
> struct net *net = xs_net(x);
> struct sk_buff *skb;
> @@ -2614,7 +2614,7 @@ static int xfrm_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *xt,
> if (skb == NULL)
> return -ENOMEM;
>
> - if (build_acquire(skb, x, xt, xp, dir) < 0)
> + if (build_acquire(skb, x, xt, xp, XFRM_POLICY_OUT) < 0)
> BUG();
xfrm_send_acquire() is the only caller of build_acquire().
So if you remove the dir parameter from xfrm_send_acquire(),
you can remove it from build_acquire() too.
^ permalink raw reply
* Re: [PATCH] XFRM: remove redundant parameter "int dir" in struct xfrm_mgr.acquire
From: Fan Du @ 2012-08-13 7:23 UTC (permalink / raw)
To: Steffen Klassert; +Cc: davem, netdev
In-Reply-To: <20120813070841.GP1869@secunet.com>
On 2012年08月13日 15:08, Steffen Klassert wrote:
> On Mon, Aug 13, 2012 at 02:25:57PM +0800, Fan Du wrote:
>>
>> static int xfrm_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *xt,
>> - struct xfrm_policy *xp, int dir)
>> + struct xfrm_policy *xp)
>> {
>> struct net *net = xs_net(x);
>> struct sk_buff *skb;
>> @@ -2614,7 +2614,7 @@ static int xfrm_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *xt,
>> if (skb == NULL)
>> return -ENOMEM;
>>
>> - if (build_acquire(skb, x, xt, xp, dir)< 0)
>> + if (build_acquire(skb, x, xt, xp, XFRM_POLICY_OUT)< 0)
>> BUG();
>
> xfrm_send_acquire() is the only caller of build_acquire().
> So if you remove the dir parameter from xfrm_send_acquire(),
> you can remove it from build_acquire() too.
>
Yep, looks like we can only remove "dir" at build_acquire, not into
copy_to_user_policy anymore :)
I will adopt your approach in v2 if Dave say *YES* about this patch.
thanks anyway.
--
Love each day!
--fan
^ permalink raw reply
* Re: [Question]About KVM network zero-copy feature!
From: Michael S. Tsirkin @ 2012-08-13 7:18 UTC (permalink / raw)
To: Peter Huang(Peng)
Cc: xiaohui.xin, kvm, netdev, virtualization, avi, Stephen Hemminger
In-Reply-To: <50285766.50307@huawei.com>
Last patchset version was this:
[PATCH v16 00/17] Provide a zero-copy method on KVM virtio-net
IIRC the main issue was the need to integrate the patchset with
macvtap (as opposed to using a separate device).
On Mon, Aug 13, 2012 at 09:24:54AM +0800, Peter Huang(Peng) wrote:
> Hi, Michael
>
> IIt will be usefull if we can implement rx zero-copy, could you give
> me some help if you know some technical details or some
> references on the internet?
>
> I am wondering may be I can take a deep look on it first then decide
> if I can take it over or not.
>
> Thanks a lot.
>
> On 2012/8/12 17:37, Michael S. Tsirkin wrote:
> > On Sat, Aug 11, 2012 at 08:42:44PM -0400, Robert Vineyard wrote:
> >> (adding Xin Xiaohui to the conversation for comment)
> >>
> >> According to the NetworkingTodo page on the KVM wiki, zero-copy RX
> >> for macvtap is in fact on the roadmap, assigned to Xin:
> >>
> >> http://www.linux-kvm.org/page/NetworkingTodo
> > AFAIK Xin left Intel and is not working on it.
> > Contributions are welcome.
> >
^ permalink raw reply
* [PATCH][XFRM][v3] Replace rwlock on xfrm_policy_afinfo with rcu
From: Priyanka Jain @ 2012-08-13 7:22 UTC (permalink / raw)
To: davem, netdev; +Cc: Priyanka Jain
xfrm_policy_afinfo is read mosly data structure.
Write on xfrm_policy_afinfo is done only at the
time of configuration.
So rwlocks can be safely replaced with RCU.
RCUs usage optimizes the performance.
Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
---
Changes for v3:
Replaced 'local_bh_disable;rcu_read_lock' with 'rcu_read_lock_bh' as
suggested by David
Changes for v2:
Re-spined to netdev-next & corrected indentation as suggested by David
For IPSEC fwd test
-On p4080ds (8-core, SMP system)
Around 110% throughput increase in case of PREEMPT_RT enabled
Around 5-6% throughput increase in case of PREEMPT_RT disabled
-On p2020 (2-core, SMP system)
Around 4-5% throughput increase in case of PREEMPT_RT disabled
net/xfrm/xfrm_policy.c | 39 +++++++++++++++++++++------------------
1 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index c5a5165..5ad4d2c 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -42,13 +42,14 @@ static DEFINE_SPINLOCK(xfrm_policy_sk_bundle_lock);
static struct dst_entry *xfrm_policy_sk_bundles;
static DEFINE_RWLOCK(xfrm_policy_lock);
-static DEFINE_RWLOCK(xfrm_policy_afinfo_lock);
-static struct xfrm_policy_afinfo *xfrm_policy_afinfo[NPROTO];
+static DEFINE_SPINLOCK(xfrm_policy_afinfo_lock);
+static struct xfrm_policy_afinfo __rcu *xfrm_policy_afinfo[NPROTO]
+ __read_mostly;
static struct kmem_cache *xfrm_dst_cache __read_mostly;
static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family);
-static void xfrm_policy_put_afinfo(struct xfrm_policy_afinfo *afinfo);
+static inline void xfrm_policy_put_afinfo(struct xfrm_policy_afinfo *afinfo);
static void xfrm_init_pmtu(struct dst_entry *dst);
static int stale_bundle(struct dst_entry *dst);
static int xfrm_bundle_ok(struct xfrm_dst *xdst);
@@ -2418,7 +2419,7 @@ int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo)
return -EINVAL;
if (unlikely(afinfo->family >= NPROTO))
return -EAFNOSUPPORT;
- write_lock_bh(&xfrm_policy_afinfo_lock);
+ spin_lock_bh(&xfrm_policy_afinfo_lock);
if (unlikely(xfrm_policy_afinfo[afinfo->family] != NULL))
err = -ENOBUFS;
else {
@@ -2439,9 +2440,9 @@ int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo)
dst_ops->neigh_lookup = xfrm_neigh_lookup;
if (likely(afinfo->garbage_collect == NULL))
afinfo->garbage_collect = xfrm_garbage_collect_deferred;
- xfrm_policy_afinfo[afinfo->family] = afinfo;
+ rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family], afinfo);
}
- write_unlock_bh(&xfrm_policy_afinfo_lock);
+ spin_unlock_bh(&xfrm_policy_afinfo_lock);
rtnl_lock();
for_each_net(net) {
@@ -2474,13 +2475,14 @@ int xfrm_policy_unregister_afinfo(struct xfrm_policy_afinfo *afinfo)
return -EINVAL;
if (unlikely(afinfo->family >= NPROTO))
return -EAFNOSUPPORT;
- write_lock_bh(&xfrm_policy_afinfo_lock);
+ spin_lock_bh(&xfrm_policy_afinfo_lock);
if (likely(xfrm_policy_afinfo[afinfo->family] != NULL)) {
if (unlikely(xfrm_policy_afinfo[afinfo->family] != afinfo))
err = -EINVAL;
else {
struct dst_ops *dst_ops = afinfo->dst_ops;
- xfrm_policy_afinfo[afinfo->family] = NULL;
+ rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
+ NULL);
dst_ops->kmem_cachep = NULL;
dst_ops->check = NULL;
dst_ops->negative_advice = NULL;
@@ -2488,7 +2490,8 @@ int xfrm_policy_unregister_afinfo(struct xfrm_policy_afinfo *afinfo)
afinfo->garbage_collect = NULL;
}
}
- write_unlock_bh(&xfrm_policy_afinfo_lock);
+ spin_unlock_bh(&xfrm_policy_afinfo_lock);
+ synchronize_rcu();
return err;
}
EXPORT_SYMBOL(xfrm_policy_unregister_afinfo);
@@ -2497,16 +2500,16 @@ static void __net_init xfrm_dst_ops_init(struct net *net)
{
struct xfrm_policy_afinfo *afinfo;
- read_lock_bh(&xfrm_policy_afinfo_lock);
- afinfo = xfrm_policy_afinfo[AF_INET];
+ rcu_read_lock_bh();
+ afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET]);
if (afinfo)
net->xfrm.xfrm4_dst_ops = *afinfo->dst_ops;
#if IS_ENABLED(CONFIG_IPV6)
- afinfo = xfrm_policy_afinfo[AF_INET6];
+ afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET6]);
if (afinfo)
net->xfrm.xfrm6_dst_ops = *afinfo->dst_ops;
#endif
- read_unlock_bh(&xfrm_policy_afinfo_lock);
+ rcu_read_unlock_bh();
}
static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
@@ -2514,16 +2517,16 @@ static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
struct xfrm_policy_afinfo *afinfo;
if (unlikely(family >= NPROTO))
return NULL;
- read_lock(&xfrm_policy_afinfo_lock);
- afinfo = xfrm_policy_afinfo[family];
+ rcu_read_lock();
+ afinfo = rcu_dereference(xfrm_policy_afinfo[family]);
if (unlikely(!afinfo))
- read_unlock(&xfrm_policy_afinfo_lock);
+ rcu_read_unlock();
return afinfo;
}
-static void xfrm_policy_put_afinfo(struct xfrm_policy_afinfo *afinfo)
+static inline void xfrm_policy_put_afinfo(struct xfrm_policy_afinfo *afinfo)
{
- read_unlock(&xfrm_policy_afinfo_lock);
+ rcu_read_unlock();
}
static int xfrm_dev_event(struct notifier_block *this, unsigned long event, void *ptr)
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH 0/4 v3 net-next] tg3: Add hwmon support
From: Benjamin Herrenschmidt @ 2012-08-13 7:37 UTC (permalink / raw)
To: David Miller; +Cc: mchan, netdev
In-Reply-To: <20120716.231128.1113699431084132190.davem@davemloft.net>
On Mon, 2012-07-16 at 23:11 -0700, David Miller wrote:
> From: "Michael Chan" <mchan@broadcom.com>
> Date: Mon, 16 Jul 2012 19:23:58 -0700
>
> > David, I've removed the binary sysfs attribute and now use
> > hwmon only. Please consider this patchset for net-next.
>
> Applied, thanks Michael.
>
> You might want to add some Kconfig logic so that it's easier
> to get the hwmon stuff automatically when tg3 is selected.
Right, just got bitten by that one with one of my test configs:
CONFIG_TIGON3=y
CONFIG_HWMON=m
results in:
drivers/built-in.o: In function `.tg3_close':
tg3.c:(.text+0x1ab664): undefined reference to `.hwmon_device_unregister'
drivers/built-in.o: In function `.tg3_hwmon_open':
tg3.c:(.text+0x1b153c): undefined reference to `.hwmon_device_register'
I hit a similar one a while back with some DRM driver, there's no
nice way to solve them afaik, other than basically forcing HWMON to
y if TIGON3 is enabled. Either that, or having TIGON3's hwmon support
be a sub-option itself dependent on CONFIG_HWMON & potentially build
as a separate module etc... quite a pain.
Cheers,
Ben.
^ permalink raw reply
* [PATCH net] net: ipv6: proc: Fix error handling
From: igorm @ 2012-08-13 8:31 UTC (permalink / raw)
To: netdev; +Cc: davem, Igor Maravic
From: Igor Maravic <igorm@etf.rs>
Fix error handling in case making of dir dev_snmp6 failes
Signed-off-by: Igor Maravic <igorm@etf.rs>
---
net/ipv6/proc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv6/proc.c b/net/ipv6/proc.c
index da2e92d..745a320 100644
--- a/net/ipv6/proc.c
+++ b/net/ipv6/proc.c
@@ -307,10 +307,10 @@ static int __net_init ipv6_proc_init_net(struct net *net)
goto proc_dev_snmp6_fail;
return 0;
+proc_dev_snmp6_fail:
+ proc_net_remove(net, "snmp6");
proc_snmp6_fail:
proc_net_remove(net, "sockstat6");
-proc_dev_snmp6_fail:
- proc_net_remove(net, "dev_snmp6");
return -ENOMEM;
}
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH V2 09/12] net/eipoib: Add main driver functionality
From: Or Gerlitz @ 2012-08-13 8:33 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Michael S. Tsirkin, davem, roland, netdev, ali, sean.hefty,
Erez Shitrit, Doug Ledford
In-Reply-To: <877gt415lu.fsf@xmission.com>
On 12/08/2012 18:40, Eric W. Biederman wrote:
> Let me give you a non-hack recomendation.
>
> - Give up on being wire compatible with IPoIB.
>
> - Define and implement ethernet over inifiniband aka EoIB.
>
> With EoIB:
> - The SM would map ethernet address to inifiniband hardware addresses.
> - You discover which multicast addresses are of interest from the
> IP layer above so no snooping is necessary.
> - You could run queue pairs directly to hosts.
>
> Shrug. It is trivial and it will work. It will probably run into the
> same problems that have historically been a problem for using IPoIB
> (lack of stateless offloads) but shrug that is mostly a NIC firmware
> problem. The switches will have no trouble and interoperability will
> be assured.
>
> If you want to map ethernet over infiniband please map ethernet over
> infiniband. Don't poorly NAT ethernet into infiniband.
>
>
EoIB is a valid suggestion and we will look into it as well, BUT:
Providing EoIB is a separate discussion, obviously defining and
standardizing a new protocol takes what is takes (a lot of time, longish
term effort), and will also take time to develop/debug/mature e.g as you
mentioned, some of the features/offloads might require new NIC HW, etc
-- compared to IPoIB which is here for many years
In practice there is already a huge install base for IPoIB software and
hardware products, in different operating environments/OS. We can't just
through away everything and tell people to replace it all with a new
protocol, e.g. bridging devices, storage systems/appliances, VMware,
Windows, .. systems in production environments --- so
the interoperability concern you've mentioned gonna hit very hard.
The eIPoIB driver comes to provide a way to work with IPoIB in
virtualized environments, where still, the suggestions/concerns raised
in this thread should be addressed.
Or.
^ permalink raw reply
* [PATCH] net: ipv4: fib_trie: Don't unnecessarily search for already found fib leaf
From: igorm @ 2012-08-13 8:26 UTC (permalink / raw)
To: netdev; +Cc: davem, Igor Maravic
From: Igor Maravic <igorm@etf.rs>
We've already found leaf, don't search for it again. Same is for fib leaf info.
Signed-off-by: Igor Maravic <igorm@etf.rs>
---
net/ipv4/fib_trie.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 30b88d7..8063a8c 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -1657,7 +1657,12 @@ int fib_table_delete(struct fib_table *tb, struct fib_config *cfg)
if (!l)
return -ESRCH;
- fa_head = get_fa_head(l, plen);
+ li = find_leaf_info(l, plen);
+
+ if (!li)
+ return -ESRCH;
+
+ fa_head = &li->falh;
fa = fib_find_alias(fa_head, tos, 0);
if (!fa)
@@ -1693,9 +1698,6 @@ int fib_table_delete(struct fib_table *tb, struct fib_config *cfg)
rtmsg_fib(RTM_DELROUTE, htonl(key), fa, plen, tb->tb_id,
&cfg->fc_nlinfo, 0);
- l = fib_find_node(t, key);
- li = find_leaf_info(l, plen);
-
list_del_rcu(&fa->fa_list);
if (!plen)
--
1.7.9.5
^ permalink raw reply related
* Re: [flame^Wreview] net: netprio_cgroup: rework update socket logic
From: Joe Perches @ 2012-08-13 9:08 UTC (permalink / raw)
To: John Fastabend; +Cc: Al Viro, netdev, David Miller, Neil Horman, linux-kernel
In-Reply-To: <502896C5.7080303@intel.com>
On Sun, 2012-08-12 at 22:55 -0700, John Fastabend wrote:
> On 8/12/2012 6:53 PM, Al Viro wrote:
> > Ladies and gentlemen, who the devil had reviewed that little gem?
> >
> > commit 406a3c638ce8b17d9704052c07955490f732c2b8
> > Author: John Fastabend <john.r.fastabend@intel.com>
> > Date: Fri Jul 20 10:39:25 2012 +0000
[]
> OK clearly I screwed it up thanks for reviewing Al. How about this.
>
> fdt = files_fdtable(files);
> for (fd = 0; fd < fdt->max_fds; fd++) {
> struct socket *sock;
> int err = 0;
Don't need to initialize err if you're not using it.
> sock = sockfd_lookup(fd, &err);
> if (!sock) {
Of course you mean
if (sock)
> lock_sock(sock->sk);
> sock_update_netprioidx(sock->sk, p);
> release_sock(sock->sk);
> sockfd_put(sock);
> }
> }
^ permalink raw reply
* Re: [PATCH v5 00/15] some netpoll and netconsole fixes
From: Cong Wang @ 2012-08-13 9:45 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller
In-Reply-To: <1344597891-32242-1-git-send-email-amwang@redhat.com>
Hi, David,
Does this patchset look good to you now?
Thanks!
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: Ian Molton @ 2012-08-13 10:00 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arm-kernel, thomas.petazzoni, andrew, netdev,
devicetree-discuss, ben.dooks, dale, linuxppc-dev, David Miller
In-Reply-To: <201208101049.57586.arnd@arndb.de>
On 10/08/12 11:49, Arnd Bergmann wrote:
> On Thursday 09 August 2012, Ian Molton wrote:
>>> The driver
>>> already knows all those offsets and they are always the same
>>> for all variants of mv643xx, right?
>> Yes, but its not clean. And no amount of refactoring is
>> really going to make a nice driver that also fits the ancient
>> (and badly thought out) OF bindings.
> In what way is it badly though out, or not clean? The use of
> underscores in the properties, and the way that the sram
> is configured is problematic, I agree. But The way that
> the three ports are addressed and how the PHY is found
> seems quite clever.
It forces one to load the MDIO driver first, because it maps ALL the
registers for both itself and all the ports, and the MDIO driver has no
way of knowing how many ethernet blocks are present (I have a device
here with two, and another with four). Thats anywhere from 1 to 12
ports, split across 1 to 4 address ranges, and theres a big gap in the
address range between controllers 0,1 and 2,3. *ALL* the devices on the
board are sharing ethernet block 0's MDIO bus. By pure luck it happens
to work, because the blocks 2,3 have an alias of the MDIO registers from
blocks 0,1.
Having the MDIO driver map the ethernet drivers memory is a terrible
solution, IMO. Ethernet drivers should map their own memory, and that
introduces the n-ports-per-block problem, because their address ranges
overlap.
I think the best solution is to make each ethernet block register 3 ports.
the PPC code can simply generate different fixups so that instead of
creating 3 devices, it creates one, with three ports.
>> If we have to break things, we can at least go for a nice
>> clean design, surely?
>>
>> The ports arent really child devices of the MAC. The MAC
>> just has 3 ports.
> I don't see the difference between those two things.
The ports are at best 'pseudodevices'. Real devices have registers of
their own.
>> We're going to have to maintain a legacy
>> platform_device -> DT bindings hack somewhere anyway,
>> at least until the remaining other users of the driver
>> convert to D-T.
> I don't understand why you describe the method used in
> powerpc as a hack. It was the normal way to introduce
> DT support for platform devices back when it was implemented.
Just because its normal doesn't mean its not a hack :)
> It also had the advantage of not requiring any modifications
> to the generic driver, because it was shared between one
> architecture using DT (powerpc) and one that didn't (ARM).
It /did/ spawn a pretty hideous driver, though...
-Ian
^ permalink raw reply
* Re: [PATCH] netvm: check for page == NULL when propogating the skb->pfmemalloc flag
From: Mel Gorman @ 2012-08-13 10:26 UTC (permalink / raw)
To: David Miller
Cc: linux-mm, linux-kernel, netdev, xen-devel, konrad, Ian.Campbell,
Jeremy Fitzhardinge, akpm
In-Reply-To: <20120808.155046.820543563969484712.davem@davemloft.net>
On Wed, Aug 08, 2012 at 03:50:46PM -0700, David Miller wrote:
> From: Mel Gorman <mgorman@suse.de>
> Date: Tue, 7 Aug 2012 09:55:55 +0100
>
> > Commit [c48a11c7: netvm: propagate page->pfmemalloc to skb] is responsible
> > for the following bug triggered by a xen network driver
> ...
> > The problem is that the xenfront driver is passing a NULL page to
> > __skb_fill_page_desc() which was unexpected. This patch checks that
> > there is a page before dereferencing.
> >
> > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Signed-off-by: Mel Gorman <mgorman@suse.de>
>
> That call to __skb_fill_page_desc() in xen-netfront.c looks completely bogus.
> It's the only driver passing NULL here.
>
> That whole song and dance figuring out what to do with the head
> fragment page, depending upon whether the length is greater than the
> RX_COPY_THRESHOLD, is completely unnecessary.
>
> Just use something like a call to __pskb_pull_tail(skb, len) and all
> that other crap around that area can simply be deleted.
I looked at this for a while but I did not see how __pskb_pull_tail()
could be used sensibly but I'm simily not familiar with writing network
device drivers or Xen.
This messing with RX_COPY_THRESHOLD seems to be related to how the frontend
and backend communicate (maybe some fixed limitation of the xenbus). The
existing code looks like it is trying to take the fragments received and
pass them straight to the backend without copying by passing the fragments
to the backend without copying. I worry that if I try converting this to
__pskb_pull_tail() that it would either hit the limitation of xenbus or
introduce copying where it is not wanted.
I'm going to have to punt this to Jeremy and the other Xen folk as I'm not
sure what the original intention was and I don't have a Xen setup anywhere
to test any patch. Jeremy, xen folk?
--
Mel Gorman
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH net 0/3] qmi_wwan: simplify device matching and add a few new devices
From: Bjørn Mork @ 2012-08-13 10:39 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA; +Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1344798992-10176-1-git-send-email-bjorn-yOkvZcmFvRU@public.gmane.org>
Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> writes:
> The home cooked whitelisting code can be removed now that
> the USB core supports interface number matching.
>
> The second patch adds a few new devices.
>
> The third patch improves device list readability by using
> existing macros where possible.
>
> I hope this can go in 3.6-rc2/3. The series is based on
> the current net/master (commit 2359a476)
Just an additional afterthought: Please do not apply this as it is to
"net-next" if you decide it is inappropriate for "net".
New devices will be added to the driver during the 3.6 cycle whether or
not the probing changes are accepted. This will result in unnecessary
merge conflicts. I'll submit a new version of the patch set for
"net-next" when we have a better idea of what the device list looks like
at the beginning of the 3.7 cycle.
But I am still hoping this is accepted for "net", although I obviously
screwed up choosing strategy to avoid net/usb merge conflicts.
Bjørn
--
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: [PATCH] netvm: check for page == NULL when propogating the skb->pfmemalloc flag
From: Mel Gorman @ 2012-08-13 10:47 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: linux-mm, linux-kernel, netdev, xen-devel, konrad, Ian.Campbell,
David Miller, akpm
In-Reply-To: <20120813102604.GC4177@suse.de>
Resending to correct Jeremy's address.
On Wed, Aug 08, 2012 at 03:50:46PM -0700, David Miller wrote:
> From: Mel Gorman <mgorman@suse.de>
> Date: Tue, 7 Aug 2012 09:55:55 +0100
>
> > Commit [c48a11c7: netvm: propagate page->pfmemalloc to skb] is responsible
> > for the following bug triggered by a xen network driver
> ...
> > The problem is that the xenfront driver is passing a NULL page to
> > __skb_fill_page_desc() which was unexpected. This patch checks that
> > there is a page before dereferencing.
> >
> > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Signed-off-by: Mel Gorman <mgorman@suse.de>
>
> That call to __skb_fill_page_desc() in xen-netfront.c looks completely bogus.
> It's the only driver passing NULL here.
>
> That whole song and dance figuring out what to do with the head
> fragment page, depending upon whether the length is greater than the
> RX_COPY_THRESHOLD, is completely unnecessary.
>
> Just use something like a call to __pskb_pull_tail(skb, len) and all
> that other crap around that area can simply be deleted.
I looked at this for a while but I did not see how __pskb_pull_tail()
could be used sensibly but I'm simily not familiar with writing network
device drivers or Xen.
This messing with RX_COPY_THRESHOLD seems to be related to how the frontend
and backend communicate (maybe some fixed limitation of the xenbus). The
existing code looks like it is trying to take the fragments received and
pass them straight to the backend without copying by passing the fragments
to the backend without copying. I worry that if I try converting this to
__pskb_pull_tail() that it would either hit the limitation of xenbus or
introduce copying where it is not wanted.
I'm going to have to punt this to Jeremy and the other Xen folk as I'm not
sure what the original intention was and I don't have a Xen setup anywhere
to test any patch. Jeremy, xen folk?
--
Mel Gorman
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [flame^Wreview] net: netprio_cgroup: rework update socket logic
From: Neil Horman @ 2012-08-13 11:22 UTC (permalink / raw)
To: John Fastabend; +Cc: Al Viro, netdev, David Miller, linux-kernel
In-Reply-To: <502896C5.7080303@intel.com>
On Sun, Aug 12, 2012 at 10:55:17PM -0700, John Fastabend wrote:
> On 8/12/2012 6:53 PM, Al Viro wrote:
> > Ladies and gentlemen, who the devil had reviewed that little gem?
> >
> >commit 406a3c638ce8b17d9704052c07955490f732c2b8
> >Author: John Fastabend <john.r.fastabend@intel.com>
> >Date: Fri Jul 20 10:39:25 2012 +0000
> >
> >is a bleeding bogosity that doesn't pass even the most cursory
> >inspection. It iterates through descriptor tables of a bunch
> >of processes, doing this:
> > file = fcheck_files(files, fd);
> > if (!file)
> > continue;
> >
> > path = d_path(&file->f_path, tmp, PAGE_SIZE);
> > rv = sscanf(path, "socket:[%lu]", &s);
> > if (rv <= 0)
> > continue;
> >
> > sock = sock_from_file(file, &err);
> > if (!err)
> > sock_update_netprioidx(sock->sk, p);
> >Note the charming use of sscanf() for pattern-matching. 's' (inode
> >number of socket) is completely unused afterwards; what happens here
> >is a very badly written attempt to skip non-sockets. Why, will
> >sock_from_file() blow up on non-sockets? And isn't there some less
> >obnoxious way to check that the file is a sockfs one? Let's see:
> >struct socket *sock_from_file(struct file *file, int *err)
> >{
> > if (file->f_op == &socket_file_ops)
> > return file->private_data; /* set in sock_map_fd */
> >
> > *err = -ENOTSOCK;
> > return NULL;
> >}
> >... and the first line is exactly that - a check that we are on sockfs.
> >_Far_ less expensive one, at that, so it's not even that we are avoiding
> >a costly test. In other words, all masturbation with d_path() is absolutely
> >pointless.
> >
> >Furthermore, it's racy; had been even more so before the delayed fput series
> >went in, but even now it's not safe. fcheck_files() does *NOT* guarantee
> >that file is not getting closed right now. rcu_read_lock() prevents only
> >freeing and potential reuse of struct file we'd got; it might go all the
> >way through final fput() just as we look at it. So file->f_path is not
> >protected by anything. Worse yet, neither is struct socket itself - we
> >might be going through sock_release() at the same time, so sock->sk might
> >very well be NULL, leaving us a oops even after we dump d_path() idiocy.
> >
> >To make it even funnier, there's such thing as SCM_RIGHTS datagrams and
> >descriptor passing. In other words, it's *not* going to catch all sockets
> >that would be caught by the earlier variant.
> >
>
Yes, thank you Al, I should have reviewed this more throughly.
> OK clearly I screwed it up thanks for reviewing Al. How about this.
>
> fdt = files_fdtable(files);
> for (fd = 0; fd < fdt->max_fds; fd++) {
> struct socket *sock;
> int err = 0;
>
> sock = sockfd_lookup(fd, &err);
> if (!sock) {
> lock_sock(sock->sk);
What are you protecting with lock_sock here? The other call sites don't make
use of the socket lock. Arguagbly they could, but I don't think they need to.
As long as the fd doesn't go away we should be able to update the
sk_cgrp_prioidx safely.
Regards
Neil
^ 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