* Re: [PATCH] per-cgroup tcp buffer limitation
From: Greg Thelen @ 2011-09-07 21:35 UTC (permalink / raw)
To: Glauber Costa
Cc: linux-kernel, linux-mm, containers, netdev, xemul,
David S. Miller, Hiroyouki Kamezawa, Eric W. Biederman,
Suleiman Souhlal
In-Reply-To: <4E66A0A9.3060403@parallels.com>
On Tue, Sep 6, 2011 at 3:37 PM, Glauber Costa <glommer@parallels.com> wrote:
> I think memcg's usage is really all you need here. In the end of the day, it
> tells you how many pages your container has available. The whole
> point of kmem cgroup is not any kind of reservation or accounting.
The memcg does not reserve memory. It provides upper bound limits on
memory usage. A careful admin can configure soft_limit_in_bytes as an
approximation of a memory reservation. But the soft limit is really
more like a reclaim target when there is global memory pressure.
> Once a container (or cgroup) reaches a number of objects *pinned* in memory
> (therefore, non-reclaimable), you won't be able to grab anything from it.
>
>> So
>> far my use cases involve a single memory limit which includes both
>> kernel and user memory. So I would need a user space agent to poll
>> {memcg,kmem}.usage_in_bytes to apply pressure to memcg if kmem grows
>> and visa versa.
>
> Maybe not.
> If userspace memory works for you today (supposing it does), why change?
Good question. Current upstream memcg user space memory limit does
not work for me today. I should have made that more obvious (sorry).
See below for details.
> Right now you assign X bytes of user memory to a container, and the kernel
> memory is shared among all of them. If this works for you, kmem_cgroup won't
> change that. It just will impose limits over which
> your kernel objects can't grow.
>
> So you don't *need* a userspace agent doing this calculation, because
> fundamentally, nothing changed: I am not unbilling memory in memcg to bill
> it back in kmem_cg. Of course, once it is in, you will be able to do it in
> such a fine grained fashion if you decide to do so.
>
>> Do you foresee instantiation of multiple kmem cgroups, so that a
>> process could be added into kmem/K1 or kmem/K2? If so do you plan on
>> supporting migration between cgroups and/or migration of kmem charge
>> between K1 to K2?
>
> Yes, each container should have its own cgroup, so at least in the use
> cases I am concerned, we will have a lot of them. But the usual lifecycle,
> is create, execute and die. Mobility between them
> is not something I am overly concerned right now.
>
>
>>>> Do you foresee the kmem cgroup growing to include reclaimable slab,
>>>> where freeing one type of memory allows for reclaim of the other?
>>>
>>> Yes, absolutely.
Now I see that you're using kmem to limit the amount of unreclaimable
kernel memory.
We have a work-in-progress patch series that adds kernel memory accounting to
memcg. These patches allow an admin to specify a single memory limit
for a cgroup which encompasses both user memory (as upstream memcg
does) and also includes many kernel memory allocations (especially
slab, page-tables). When kernel memory grows it puts pressure on user
memory; when user memory grows it puts pressure on reclaimable kernel
memory using registered shrinkers. We are in the process of cleaning
up these memcg slab accounting patches.
In my uses cases there is a single memory limit that applies to both
kernel and user memory. If a separate kmem cgroup is introduced to
manage kernel memory outside of memcg with a distinct limit, then I
would need a user space daemon which balances memory between the kmem
and memcg subsystems. As kmem grows, this daemon would apply pressure
to memcg, and as memcg grows pressure would be applied to kmem. As
you stated kernel memory is not necessarily reclaimable. So such
reclaim may fail. My resistance to this approach is that with a
single memory cgroup admins can do a better job packing a machine. If
balancing daemons are employed then more memory would need to be
reserved and more user space cpu time would be needed to apply VM
pressure between the types of memory.
While there are people (like me) who want a combined memory usage
limit there are also people (like you) who want separate user and
kernel limiting. I have toyed with the idea of having a per cgroup
flag that determines if kernel and user memory should be combined
charged against a single limit or if they should have separate limits.
I have also wondered if there was a way to wire the usage of two
subsystems together, then it would also meet meet my needs. But I am
not sure how to do that.
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH -next v2] unix stream: Fix use-after-free crashes
From: Tim Chen @ 2011-09-07 14:37 UTC (permalink / raw)
To: sedat.dilek
Cc: Eric Dumazet, Yan, Zheng, Yan, Zheng, netdev@vger.kernel.org,
davem@davemloft.net, sfr@canb.auug.org.au, jirislaby@gmail.com,
Shi, Alex, Valdis Kletnieks
In-Reply-To: <CA+icZUW5Biu8cMhaJcz=MwtMOVn-NFD92PTpT0-DfEDKrRmaUQ@mail.gmail.com>
On Wed, 2011-09-07 at 22:30 +0200, Sedat Dilek wrote:
> >
> > Replaced v2 with this patch (against next-20110831), I see now some
> > different call-traces which I did not see with v1 or v2.
> > Can't say if it's related to the new patch or not.
> > ( dmesg attached. )
> >
> > - Sedat -
> >
>
> Call-traces seem to go away when adding "irqpoll" to Kernel command line.
> ( See dmesg_irqpoll.txt )
>
> - Sedat -
Sedat,
The previous patch should use the new steal_refs to check for the
release of scm references in the error handling at the end. I've
updated the patch to take care of it. Hopefully the traces you see will
go away. Can you verify?
Thanks.
Tim
----
Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
in Unix socket's send and receive path) introduced a use-after-free bug.
The sent skbs from unix_stream_sendmsg could be consumed and destructed
by the receive side, removing all referentials to the credentials,
before the send side has finished sending out all
packets. However, send side could continue to consturct new packets in the
stream, using credentials that have lost its last reference and been
freed.
In this fix, we don't steal the reference to credentials we have obtained
in scm_send at beginning of unix_stream_sendmsg, till we've reached
the last packet. This fixes the problem in commit 0856a30409.
Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Reported-by: Jiri Slaby <jirislaby@gmail.com>
Tested-by: Sedat Dilek <sedat.dilek@googlemail.com>
Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
---
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 136298c..be712ae 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1383,10 +1383,11 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
}
static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb,
- bool send_fds, bool ref)
+ bool send_fds, bool steal_refs)
{
int err = 0;
- if (ref) {
+
+ if (!steal_refs) {
UNIXCB(skb).pid = get_pid(scm->pid);
UNIXCB(skb).cred = get_cred(scm->cred);
} else {
@@ -1458,7 +1459,7 @@ static int unix_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock,
if (skb == NULL)
goto out;
- err = unix_scm_to_skb(siocb->scm, skb, true, false);
+ err = unix_scm_to_skb(siocb->scm, skb, true, true);
if (err < 0)
goto out_free;
max_level = err + 1;
@@ -1581,6 +1582,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
int sent = 0;
struct scm_cookie tmp_scm;
bool fds_sent = false;
+ bool steal_refs = false;
int max_level;
if (NULL == siocb->scm)
@@ -1642,11 +1644,14 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
size = min_t(int, size, skb_tailroom(skb));
- /* Only send the fds and no ref to pid in the first buffer */
- err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
+ /* Only send the fds in first buffer
+ * Last buffer can steal our references to pid/cred
+ */
+ steal_refs = (sent + size >= len);
+ err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, steal_refs);
if (err < 0) {
kfree_skb(skb);
- goto out;
+ goto out_err;
}
max_level = err + 1;
fds_sent = true;
@@ -1654,7 +1659,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
err = memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size);
if (err) {
kfree_skb(skb);
- goto out;
+ goto out_err;
}
unix_state_lock(other);
@@ -1671,7 +1676,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
sent += size;
}
- if (skb)
+ if (steal_refs)
scm_release(siocb->scm);
else
scm_destroy(siocb->scm);
@@ -1687,9 +1692,8 @@ pipe_err:
send_sig(SIGPIPE, current, 0);
err = -EPIPE;
out_err:
- if (skb == NULL)
+ if (!steal_refs)
scm_destroy(siocb->scm);
-out:
siocb->scm = NULL;
return sent ? : err;
}
^ permalink raw reply related
* [PATCH 1/1] ibmveth: Fix DMA unmap error
From: Brian King @ 2011-09-07 21:37 UTC (permalink / raw)
To: netdev; +Cc: santil, anton, brking
Commit 6e8ab30ec677925e8999a9f5bdb028736d22d48c introduced a DMA mapping
API inconsistency resulting in dma_unmap_page getting called on memory
mapped via dma_map_single. This was seen when CONFIG_DMA_API_DEBUG was
enabled. Fix up this API usage inconsistency.
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
drivers/net/ibmveth.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff -puN drivers/net/ibmveth.c~ibmveth_fix_mapping_issue drivers/net/ibmveth.c
--- linux-2.6/drivers/net/ibmveth.c~ibmveth_fix_mapping_issue 2011-09-07 16:28:19.000000000 -0500
+++ linux-2.6-bjking1/drivers/net/ibmveth.c 2011-09-07 16:28:19.000000000 -0500
@@ -1026,7 +1026,12 @@ retry_bounce:
netdev->stats.tx_bytes += skb->len;
}
- for (i = 0; i < skb_shinfo(skb)->nr_frags + 1; i++)
+ dma_unmap_single(&adapter->vdev->dev,
+ descs[0].fields.address,
+ descs[0].fields.flags_len & IBMVETH_BUF_LEN_MASK,
+ DMA_TO_DEVICE);
+
+ for (i = 1; i < skb_shinfo(skb)->nr_frags + 1; i++)
dma_unmap_page(&adapter->vdev->dev, descs[i].fields.address,
descs[i].fields.flags_len & IBMVETH_BUF_LEN_MASK,
DMA_TO_DEVICE);
_
^ permalink raw reply
* Re: [PATCH v2 3/9] socket: initial cgroup code.
From: Kirill A. Shutemov @ 2011-09-07 22:17 UTC (permalink / raw)
To: Glauber Costa
Cc: linux-kernel, xemul, netdev, linux-mm, Eric W. Biederman,
containers, David S. Miller
In-Reply-To: <1315369399-3073-4-git-send-email-glommer@parallels.com>
On Wed, Sep 07, 2011 at 01:23:13AM -0300, Glauber Costa wrote:
> We aim to control the amount of kernel memory pinned at any
> time by tcp sockets. To lay the foundations for this work,
> this patch adds a pointer to the kmem_cgroup to the socket
> structure.
>
> Signed-off-by: Glauber Costa <glommer@parallels.com>
> CC: David S. Miller <davem@davemloft.net>
> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com>
> CC: Eric W. Biederman <ebiederm@xmission.com>
> ---
> include/linux/kmem_cgroup.h | 29 +++++++++++++++++++++++++++++
> include/net/sock.h | 2 ++
> net/core/sock.c | 5 ++---
> 3 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/kmem_cgroup.h b/include/linux/kmem_cgroup.h
> index 0e4a74b..77076d8 100644
> --- a/include/linux/kmem_cgroup.h
> +++ b/include/linux/kmem_cgroup.h
> @@ -49,5 +49,34 @@ static inline struct kmem_cgroup *kcg_from_task(struct task_struct *tsk)
> return NULL;
> }
> #endif /* CONFIG_CGROUP_KMEM */
> +
> +#ifdef CONFIG_INET
Will it break something if you define the helpers even if CONFIG_INET
is not defined?
It will be much cleaner. You can reuse ifdef CONFIG_CGROUP_KMEM in this
case.
> +#include <net/sock.h>
> +static inline void sock_update_kmem_cgrp(struct sock *sk)
> +{
> +#ifdef CONFIG_CGROUP_KMEM
> + sk->sk_cgrp = kcg_from_task(current);
> +
> + /*
> + * We don't need to protect against anything task-related, because
> + * we are basically stuck with the sock pointer that won't change,
> + * even if the task that originated the socket changes cgroups.
> + *
> + * What we do have to guarantee, is that the chain leading us to
> + * the top level won't change under our noses. Incrementing the
> + * reference count via cgroup_exclude_rmdir guarantees that.
> + */
> + cgroup_exclude_rmdir(&sk->sk_cgrp->css);
> +#endif
> +}
> +
> +static inline void sock_release_kmem_cgrp(struct sock *sk)
> +{
> +#ifdef CONFIG_CGROUP_KMEM
> + cgroup_release_and_wakeup_rmdir(&sk->sk_cgrp->css);
> +#endif
> +}
> +
> +#endif /* CONFIG_INET */
> #endif /* _LINUX_KMEM_CGROUP_H */
> @@ -2252,9 +2254,6 @@ void sk_common_release(struct sock *sk)
> }
> EXPORT_SYMBOL(sk_common_release);
>
> -static DEFINE_RWLOCK(proto_list_lock);
> -static LIST_HEAD(proto_list);
> -
Wrong patch?
--
Kirill A. Shutemov
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH 01/15] add Documentation/namespaces/user_namespace.txt (v3)
From: Andrew Morton @ 2011-09-07 22:50 UTC (permalink / raw)
To: Serge Hallyn
Cc: segooon, linux-kernel, netdev, containers, dhowells, ebiederm,
rdunlap, Serge E. Hallyn
In-Reply-To: <1314993400-6910-4-git-send-email-serge@hallyn.com>
On Fri, 2 Sep 2011 19:56:26 +0000
Serge Hallyn <serge@hallyn.com> wrote:
> +Note that this userid mapping for the VFS is not yet implemented, though the
> +lkml and containers mailing list archives will show several previous
> +prototypes. In the end, those got hung up waiting on the concept of targeted
> +capabilities to be developed, which, thanks to the insight of Eric Biederman,
> +they finally did.
not-yet-implemented things worry me. When can we expect this to
happen, and how big and ugly will it be?
I'm not seeing many (any) reviewed-by's on these patches. I could get
down and stare at them myself, but that wouldn't be very useful. This
work goes pretty deep and is quite security-affecting. And network-afecting.
Can you round up some suitable people and get the reviewing and testing happening
please?
^ permalink raw reply
* [RFC] interface for outgoing packet
From: Viral Mehta @ 2011-09-08 0:24 UTC (permalink / raw)
To: netdev
Hi All,
How reasonably I can assume that,
on whatever interface, I received the last packet
for a particular Socket Connection, the same interface will be
used to SEND the next packet for same socket connection?
I am collecting the logs on one of my test machines.
And it shows, I can assume all the time that "interface" which received packet
for some socket connection, the same will be used to send packet for
that connection
But, I am not sure if I am missing some scenarios
or setup (for e.g., bonding) where it can be wrong ?
It would be more help if some one can shed some light.
--
Thanks,
Viral Mehta
^ permalink raw reply
* Re: [PATCH -next v2] unix stream: Fix use-after-free crashes
From: Yan, Zheng @ 2011-09-08 0:27 UTC (permalink / raw)
To: Tim Chen
Cc: sedat.dilek@gmail.com, Eric Dumazet, Yan, Zheng,
netdev@vger.kernel.org, davem@davemloft.net, sfr@canb.auug.org.au,
jirislaby@gmail.com, Shi, Alex, Valdis Kletnieks
In-Reply-To: <1315406256.6287.7.camel@schen9-mobl>
On 09/07/2011 10:37 PM, Tim Chen wrote:
> On Wed, 2011-09-07 at 22:30 +0200, Sedat Dilek wrote:
>
>>>
>>> Replaced v2 with this patch (against next-20110831), I see now some
>>> different call-traces which I did not see with v1 or v2.
>>> Can't say if it's related to the new patch or not.
>>> ( dmesg attached. )
>>>
>>> - Sedat -
>>>
>>
>> Call-traces seem to go away when adding "irqpoll" to Kernel command line.
>> ( See dmesg_irqpoll.txt )
>>
>> - Sedat -
>
> Sedat,
>
> The previous patch should use the new steal_refs to check for the
> release of scm references in the error handling at the end. I've
> updated the patch to take care of it. Hopefully the traces you see will
> go away. Can you verify?
>
> Thanks.
>
> Tim
>
> ----
> Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
> in Unix socket's send and receive path) introduced a use-after-free bug.
> The sent skbs from unix_stream_sendmsg could be consumed and destructed
> by the receive side, removing all referentials to the credentials,
> before the send side has finished sending out all
> packets. However, send side could continue to consturct new packets in the
> stream, using credentials that have lost its last reference and been
> freed.
>
> In this fix, we don't steal the reference to credentials we have obtained
> in scm_send at beginning of unix_stream_sendmsg, till we've reached
> the last packet. This fixes the problem in commit 0856a30409.
>
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Reported-by: Jiri Slaby <jirislaby@gmail.com>
> Tested-by: Sedat Dilek <sedat.dilek@googlemail.com>
> Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
> ---
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 136298c..be712ae 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1383,10 +1383,11 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
> }
>
> static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb,
> - bool send_fds, bool ref)
> + bool send_fds, bool steal_refs)
> {
> int err = 0;
> - if (ref) {
> +
> + if (!steal_refs) {
> UNIXCB(skb).pid = get_pid(scm->pid);
> UNIXCB(skb).cred = get_cred(scm->cred);
> } else {
> @@ -1458,7 +1459,7 @@ static int unix_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock,
> if (skb == NULL)
> goto out;
>
> - err = unix_scm_to_skb(siocb->scm, skb, true, false);
> + err = unix_scm_to_skb(siocb->scm, skb, true, true);
> if (err < 0)
> goto out_free;
> max_level = err + 1;
> @@ -1581,6 +1582,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
> int sent = 0;
> struct scm_cookie tmp_scm;
> bool fds_sent = false;
> + bool steal_refs = false;
> int max_level;
>
> if (NULL == siocb->scm)
> @@ -1642,11 +1644,14 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
> size = min_t(int, size, skb_tailroom(skb));
>
>
> - /* Only send the fds and no ref to pid in the first buffer */
> - err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
> + /* Only send the fds in first buffer
> + * Last buffer can steal our references to pid/cred
> + */
> + steal_refs = (sent + size >= len);
> + err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, steal_refs);
> if (err < 0) {
> kfree_skb(skb);
> - goto out;
> + goto out_err;
> }
> max_level = err + 1;
> fds_sent = true;
> @@ -1654,7 +1659,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
> err = memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size);
> if (err) {
> kfree_skb(skb);
> - goto out;
> + goto out_err;
> }
>
> unix_state_lock(other);
> @@ -1671,7 +1676,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
> sent += size;
> }
>
> - if (skb)
> + if (steal_refs)
> scm_release(siocb->scm);
> else
> scm_destroy(siocb->scm);
> @@ -1687,9 +1692,8 @@ pipe_err:
> send_sig(SIGPIPE, current, 0);
> err = -EPIPE;
> out_err:
> - if (skb == NULL)
> + if (!steal_refs)
> scm_destroy(siocb->scm);
I think we should call scm_release() here in the case of
steal_refs == true. Otherwise siocb->scm->fp may leak.
> -out:
> siocb->scm = NULL;
> return sent ? : err;
> }
>
>
^ permalink raw reply
* [PATCH 3/4] ibmveth: Checksum offload is always disabled
From: Anton Blanchard @ 2011-09-08 0:41 UTC (permalink / raw)
To: Santiago Leon, brking, rcj, mirq-linux; +Cc: netdev
In-Reply-To: <20110908004102.355674129@samba.org>
[-- Attachment #1: ibmveth_fix_csum.patch --]
[-- Type: text/plain, Size: 766 bytes --]
Commit b9367bf3ee6d (net: ibmveth: convert to hw_features) reversed
a check in ibmveth_set_csum_offload that results in checksum offload
never being enabled.
Signed-off-by: Anton Blanchard <anton@samba.org>
Cc: <stable@kernel.org> # 3.0+
---
Index: linux-build/drivers/net/ibmveth.c
===================================================================
--- linux-build.orig/drivers/net/ibmveth.c 2011-09-01 16:02:12.198726425 +1000
+++ linux-build/drivers/net/ibmveth.c 2011-09-01 16:05:37.282482851 +1000
@@ -812,7 +812,7 @@ static int ibmveth_set_csum_offload(stru
} else
adapter->fw_ipv6_csum_support = data;
- if (ret != H_SUCCESS || ret6 != H_SUCCESS)
+ if (ret == H_SUCCESS || ret6 == H_SUCCESS)
adapter->rx_csum = data;
else
rc1 = -EIO;
^ permalink raw reply
* [PATCH 0/4] ibmveth fixes
From: Anton Blanchard @ 2011-09-08 0:41 UTC (permalink / raw)
To: Santiago Leon, brking, rcj; +Cc: netdev
Here are a number of fixes found during ibmveth testing.
Anton
^ permalink raw reply
* [PATCH 2/4] ibmveth: Fix issue with DMA mapping failure
From: Anton Blanchard @ 2011-09-08 0:41 UTC (permalink / raw)
To: Santiago Leon, brking, rcj; +Cc: netdev
In-Reply-To: <20110908004102.355674129@samba.org>
[-- Attachment #1: ibmveth_dma_mapping_error.patch --]
[-- Type: text/plain, Size: 1604 bytes --]
descs[].fields.address is 32bit which truncates any dma mapping
errors so dma_mapping_error() fails to catch it.
Use a dma_addr_t to do the comparison. With this patch I was able
to transfer many gigabytes of data with IOMMU fault injection set
at 10% probability.
Signed-off-by: Anton Blanchard <anton@samba.org>
Cc: <stable@kernel.org> # v2.6.37+
---
Index: linux-build/drivers/net/ibmveth.c
===================================================================
--- linux-build.orig/drivers/net/ibmveth.c 2011-09-01 15:01:18.066844039 +1000
+++ linux-build/drivers/net/ibmveth.c 2011-09-01 15:03:34.299079796 +1000
@@ -930,6 +930,7 @@ static netdev_tx_t ibmveth_start_xmit(st
union ibmveth_buf_desc descs[6];
int last, i;
int force_bounce = 0;
+ dma_addr_t dma_addr;
/*
* veth handles a maximum of 6 segments including the header, so
@@ -994,17 +995,16 @@ retry_bounce:
}
/* Map the header */
- descs[0].fields.address = dma_map_single(&adapter->vdev->dev, skb->data,
- skb_headlen(skb),
- DMA_TO_DEVICE);
- if (dma_mapping_error(&adapter->vdev->dev, descs[0].fields.address))
+ dma_addr = dma_map_single(&adapter->vdev->dev, skb->data,
+ skb_headlen(skb), DMA_TO_DEVICE);
+ if (dma_mapping_error(&adapter->vdev->dev, dma_addr))
goto map_failed;
descs[0].fields.flags_len = desc_flags | skb_headlen(skb);
+ descs[0].fields.address = dma_addr;
/* Map the frags */
for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
- unsigned long dma_addr;
skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
dma_addr = dma_map_page(&adapter->vdev->dev, frag->page,
^ permalink raw reply
* [PATCH 4/4] ibmveth: Fix checksum offload failure handling
From: Anton Blanchard @ 2011-09-08 0:41 UTC (permalink / raw)
To: Santiago Leon, brking, rcj, mirq-linux; +Cc: netdev
In-Reply-To: <20110908004102.355674129@samba.org>
[-- Attachment #1: ibmveth_fix_csum2.patch --]
[-- Type: text/plain, Size: 2988 bytes --]
Fix a number of issues in ibmveth_set_csum_offload:
- set_attr6 and clr_attr6 may be used uninitialised
- We store the result of the IPV4 checksum change in ret but overwrite
it in a couple of places before checking it again later. Add ret4
to make it obvious what we are doing.
- We weren't clearing the NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM flags
if the enable of that hypervisor feature failed.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
This patch could do with some review and I haven't marked it -stable
yet.
One thing that stands out is that we need to return an error back
through set_features if any feature bit gets changed.
Index: linux-build/drivers/net/ibmveth.c
===================================================================
--- linux-build.orig/drivers/net/ibmveth.c 2011-09-01 16:06:19.000000000 +1000
+++ linux-build/drivers/net/ibmveth.c 2011-09-02 09:05:57.585353722 +1000
@@ -757,7 +757,7 @@ static int ibmveth_set_csum_offload(stru
struct ibmveth_adapter *adapter = netdev_priv(dev);
unsigned long set_attr, clr_attr, ret_attr;
unsigned long set_attr6, clr_attr6;
- long ret, ret6;
+ long ret, ret4, ret6;
int rc1 = 0, rc2 = 0;
int restart = 0;
@@ -770,6 +770,8 @@ static int ibmveth_set_csum_offload(stru
set_attr = 0;
clr_attr = 0;
+ set_attr6 = 0;
+ clr_attr6 = 0;
if (data) {
set_attr = IBMVETH_ILLAN_IPV4_TCP_CSUM;
@@ -784,16 +786,20 @@ static int ibmveth_set_csum_offload(stru
if (ret == H_SUCCESS && !(ret_attr & IBMVETH_ILLAN_ACTIVE_TRUNK) &&
!(ret_attr & IBMVETH_ILLAN_TRUNK_PRI_MASK) &&
(ret_attr & IBMVETH_ILLAN_PADDED_PKT_CSUM)) {
- ret = h_illan_attributes(adapter->vdev->unit_address, clr_attr,
+ ret4 = h_illan_attributes(adapter->vdev->unit_address, clr_attr,
set_attr, &ret_attr);
- if (ret != H_SUCCESS) {
+ if (ret4 != H_SUCCESS) {
netdev_err(dev, "unable to change IPv4 checksum "
"offload settings. %d rc=%ld\n",
- data, ret);
+ data, ret4);
+
+ h_illan_attributes(adapter->vdev->unit_address,
+ set_attr, clr_attr, &ret_attr);
+
+ if (data == 1)
+ dev->features &= ~NETIF_F_IP_CSUM;
- ret = h_illan_attributes(adapter->vdev->unit_address,
- set_attr, clr_attr, &ret_attr);
} else {
adapter->fw_ipv4_csum_support = data;
}
@@ -804,15 +810,18 @@ static int ibmveth_set_csum_offload(stru
if (ret6 != H_SUCCESS) {
netdev_err(dev, "unable to change IPv6 checksum "
"offload settings. %d rc=%ld\n",
- data, ret);
+ data, ret6);
+
+ h_illan_attributes(adapter->vdev->unit_address,
+ set_attr6, clr_attr6, &ret_attr);
+
+ if (data == 1)
+ dev->features &= ~NETIF_F_IPV6_CSUM;
- ret = h_illan_attributes(adapter->vdev->unit_address,
- set_attr6, clr_attr6,
- &ret_attr);
} else
adapter->fw_ipv6_csum_support = data;
- if (ret == H_SUCCESS || ret6 == H_SUCCESS)
+ if (ret4 == H_SUCCESS || ret6 == H_SUCCESS)
adapter->rx_csum = data;
else
rc1 = -EIO;
^ permalink raw reply
* [PATCH 1/4] ibmveth: Fix DMA unmap error
From: Anton Blanchard @ 2011-09-08 0:41 UTC (permalink / raw)
To: Santiago Leon, brking, rcj; +Cc: netdev
In-Reply-To: <20110908004102.355674129@samba.org>
[-- Attachment #1: ibmveth_1.patch --]
[-- Type: text/plain, Size: 1208 bytes --]
From: Brian King <brking@linux.vnet.ibm.com>
Commit 6e8ab30ec677 (ibmveth: Add scatter-gather support) introduced a
DMA mapping API inconsistency resulting in dma_unmap_page getting
called on memory mapped via dma_map_single. This was seen when
CONFIG_DMA_API_DEBUG was enabled. Fix up this API usage inconsistency.
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
Acked-by: Anton Blanchard <anton@samba.org>
Cc: <stable@kernel.org> # v2.6.37+
---
Index: linux-build/drivers/net/ibmveth.c
===================================================================
--- linux-build.orig/drivers/net/ibmveth.c 2011-09-08 08:00:16.842856634 +1000
+++ linux-build/drivers/net/ibmveth.c 2011-09-08 09:45:43.163851274 +1000
@@ -1026,7 +1026,12 @@ retry_bounce:
netdev->stats.tx_bytes += skb->len;
}
- for (i = 0; i < skb_shinfo(skb)->nr_frags + 1; i++)
+ dma_unmap_single(&adapter->vdev->dev,
+ descs[0].fields.address,
+ descs[0].fields.flags_len & IBMVETH_BUF_LEN_MASK,
+ DMA_TO_DEVICE);
+
+ for (i = 1; i < skb_shinfo(skb)->nr_frags + 1; i++)
dma_unmap_page(&adapter->vdev->dev, descs[i].fields.address,
descs[i].fields.flags_len & IBMVETH_BUF_LEN_MASK,
DMA_TO_DEVICE);
^ permalink raw reply
* Re: [PATCH 1/2] iwlegacy: change IWL_WARN to IWL_DEBUG_HT in iwl4965_tx_agg_start
From: Greg Dietsche @ 2011-09-08 3:51 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: linville, linux-wireless, netdev, linux-kernel
In-Reply-To: <20110906150131.GA7322@redhat.com>
Hi Stanislaw,
On 09/06/2011 10:01 AM, Stanislaw Gruszka wrote:
> I put patches here:
> http://people.redhat.com/sgruszka/iwlegacy_cleanup.tar.bz2
>
> They are on top of wireless-testing tree.
>
<snip>
> Series include your 2 patches. You can test this cleanup and
> apply your new changes on top. I'll not do any further cleanup
> for some time now, perhaps continue when I got public git tree.
>
>
Thanks! I've re-worked my patches and you can find them here:
http://www.gregd.org/stuff/linux/iwlegacy_cleanup_greg.tar.bz2
I also decided to play with github a little bit:
git://github.com/dietsche/linux.git and pushed two branches:
1) wireless-next-iwlegacy-stanislaw - your patch set
2) wireless-next-iwlegacy-stanislaw-greg - a branch that has my
additional patches.
`git format-patch
wireless-next-iwlegacy-stanislaw..wireless-next-iwlegacy-stanislaw-greg`
will generate the patches that are in the link i posted above.
The first two patches in my series are the ones that I think folks
should take a closer look at. The rest are pretty safe.
I'm running both patch sets right now on my laptop. So far it is running
perfectly.
Greg
^ permalink raw reply
* Re: [PATCH -next v2] unix stream: Fix use-after-free crashes
From: Tim Chen @ 2011-09-07 21:06 UTC (permalink / raw)
To: Yan, Zheng
Cc: sedat.dilek@gmail.com, Eric Dumazet, Yan, Zheng,
netdev@vger.kernel.org, davem@davemloft.net, sfr@canb.auug.org.au,
jirislaby@gmail.com, Shi, Alex, Valdis Kletnieks
In-Reply-To: <4E680BF1.8000901@intel.com>
On Thu, 2011-09-08 at 08:27 +0800, Yan, Zheng wrote:
> > err = -EPIPE;
> > out_err:
> > - if (skb == NULL)
> > + if (!steal_refs)
> > scm_destroy(siocb->scm);
>
> I think we should call scm_release() here in the case of
> steal_refs == true. Otherwise siocb->scm->fp may leak.
Yan Zheng,
I've updated the patch. If it looks good to you now, can you add your
Signed-off-by to the patch. Pending Sedat's testing on this patch,
I think it is good to go.
Tim
---
Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
in Unix socket's send and receive path) introduced a use-after-free bug.
The sent skbs from unix_stream_sendmsg could be consumed and destructed
by the receive side, removing all references to the credentials,
before the send side has finished sending out all
packets. However, send side could continue to consturct new packets in the
stream, using credentials that have lost its last reference and been
freed.
In this fix, we don't steal the reference to credentials we have obtained
in scm_send at beginning of unix_stream_sendmsg, till we've reached
the last packet. This fixes the problem in commit 0856a30409.
Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
Reported-by: Jiri Slaby <jirislaby@gmail.com>
Tested-by: Sedat Dilek <sedat.dilek@googlemail.com>
Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
---
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 136298c..47780dc 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1383,10 +1383,11 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
}
static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb,
- bool send_fds, bool ref)
+ bool send_fds, bool steal_refs)
{
int err = 0;
- if (ref) {
+
+ if (!steal_refs) {
UNIXCB(skb).pid = get_pid(scm->pid);
UNIXCB(skb).cred = get_cred(scm->cred);
} else {
@@ -1458,7 +1459,7 @@ static int unix_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock,
if (skb == NULL)
goto out;
- err = unix_scm_to_skb(siocb->scm, skb, true, false);
+ err = unix_scm_to_skb(siocb->scm, skb, true, true);
if (err < 0)
goto out_free;
max_level = err + 1;
@@ -1581,6 +1582,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
int sent = 0;
struct scm_cookie tmp_scm;
bool fds_sent = false;
+ bool steal_refs = false;
int max_level;
if (NULL == siocb->scm)
@@ -1642,11 +1644,14 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
size = min_t(int, size, skb_tailroom(skb));
- /* Only send the fds and no ref to pid in the first buffer */
- err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
+ /* Only send the fds in first buffer
+ * Last buffer can steal our references to pid/cred
+ */
+ steal_refs = (sent + size >= len);
+ err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, steal_refs);
if (err < 0) {
kfree_skb(skb);
- goto out;
+ goto out_err;
}
max_level = err + 1;
fds_sent = true;
@@ -1654,7 +1659,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
err = memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size);
if (err) {
kfree_skb(skb);
- goto out;
+ goto out_err;
}
unix_state_lock(other);
@@ -1671,7 +1676,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
sent += size;
}
- if (skb)
+ if (steal_refs)
scm_release(siocb->scm);
else
scm_destroy(siocb->scm);
@@ -1687,9 +1692,10 @@ pipe_err:
send_sig(SIGPIPE, current, 0);
err = -EPIPE;
out_err:
- if (skb == NULL)
+ if (steal_refs)
+ scm_release(siocb->scm);
+ else
scm_destroy(siocb->scm);
-out:
siocb->scm = NULL;
return sent ? : err;
}
^ permalink raw reply related
* Re: [PATCH -next v2] unix stream: Fix use-after-free crashes
From: Tim Chen @ 2011-09-07 21:15 UTC (permalink / raw)
To: davem@davemloft.net
Cc: sedat.dilek@gmail.com, Eric Dumazet, Yan, Zheng,
netdev@vger.kernel.org, davem@davemloft.net, sfr@canb.auug.org.au,
jirislaby@gmail.com, Shi, Alex, Valdis Kletnieks, Yan, Zheng
In-Reply-To: <1315429583.2361.3.camel@schen9-mobl>
On Wed, 2011-09-07 at 14:06 -0700, Tim Chen wrote:
> On Thu, 2011-09-08 at 08:27 +0800, Yan, Zheng wrote:
>
> > > err = -EPIPE;
> > > out_err:
> > > - if (skb == NULL)
> > > + if (!steal_refs)
> > > scm_destroy(siocb->scm);
> >
> > I think we should call scm_release() here in the case of
> > steal_refs == true. Otherwise siocb->scm->fp may leak.
>
> Yan Zheng,
>
> I've updated the patch. If it looks good to you now, can you add your
> Signed-off-by to the patch. Pending Sedat's testing on this patch,
> I think it is good to go.
>
> Tim
Oops, I've forgotten to add Eric's previous Signed-off-by in my latest
patch log. David, please add that when you pick up the patch.
Once Yan Zheng added his sign off and Sedat tested the patch, I think it
will be good to go.
Thanks.
Tim
^ permalink raw reply
* Re: [PATCH -next v2] unix stream: Fix use-after-free crashes
From: Yan, Zheng @ 2011-09-08 4:18 UTC (permalink / raw)
To: Tim Chen
Cc: sedat.dilek@gmail.com, Eric Dumazet, Yan, Zheng,
netdev@vger.kernel.org, davem@davemloft.net, sfr@canb.auug.org.au,
jirislaby@gmail.com, Shi, Alex, Valdis Kletnieks
In-Reply-To: <1315429583.2361.3.camel@schen9-mobl>
On 09/08/2011 05:06 AM, Tim Chen wrote:
> On Thu, 2011-09-08 at 08:27 +0800, Yan, Zheng wrote:
>
>>> err = -EPIPE;
>>> out_err:
>>> - if (skb == NULL)
>>> + if (!steal_refs)
>>> scm_destroy(siocb->scm);
>>
>> I think we should call scm_release() here in the case of
>> steal_refs == true. Otherwise siocb->scm->fp may leak.
>
> Yan Zheng,
>
> I've updated the patch. If it looks good to you now, can you add your
> Signed-off-by to the patch. Pending Sedat's testing on this patch,
> I think it is good to go.
>
> Tim
>
> ---
> Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
> in Unix socket's send and receive path) introduced a use-after-free bug.
> The sent skbs from unix_stream_sendmsg could be consumed and destructed
> by the receive side, removing all references to the credentials,
> before the send side has finished sending out all
> packets. However, send side could continue to consturct new packets in the
> stream, using credentials that have lost its last reference and been
> freed.
>
> In this fix, we don't steal the reference to credentials we have obtained
> in scm_send at beginning of unix_stream_sendmsg, till we've reached
> the last packet. This fixes the problem in commit 0856a30409.
>
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> Reported-by: Jiri Slaby <jirislaby@gmail.com>
> Tested-by: Sedat Dilek <sedat.dilek@googlemail.com>
> Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
> ---
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 136298c..47780dc 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1383,10 +1383,11 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
> }
>
> static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb,
> - bool send_fds, bool ref)
> + bool send_fds, bool steal_refs)
> {
> int err = 0;
> - if (ref) {
> +
> + if (!steal_refs) {
> UNIXCB(skb).pid = get_pid(scm->pid);
> UNIXCB(skb).cred = get_cred(scm->cred);
> } else {
> @@ -1458,7 +1459,7 @@ static int unix_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock,
> if (skb == NULL)
> goto out;
>
> - err = unix_scm_to_skb(siocb->scm, skb, true, false);
> + err = unix_scm_to_skb(siocb->scm, skb, true, true);
> if (err < 0)
> goto out_free;
> max_level = err + 1;
> @@ -1581,6 +1582,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
> int sent = 0;
> struct scm_cookie tmp_scm;
> bool fds_sent = false;
> + bool steal_refs = false;
> int max_level;
>
> if (NULL == siocb->scm)
> @@ -1642,11 +1644,14 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
> size = min_t(int, size, skb_tailroom(skb));
>
>
> - /* Only send the fds and no ref to pid in the first buffer */
> - err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
> + /* Only send the fds in first buffer
> + * Last buffer can steal our references to pid/cred
> + */
> + steal_refs = (sent + size >= len);
> + err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, steal_refs);
> if (err < 0) {
> kfree_skb(skb);
> - goto out;
> + goto out_err;
> }
> max_level = err + 1;
> fds_sent = true;
> @@ -1654,7 +1659,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
> err = memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size);
> if (err) {
> kfree_skb(skb);
> - goto out;
> + goto out_err;
> }
>
> unix_state_lock(other);
> @@ -1671,7 +1676,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
> sent += size;
> }
>
> - if (skb)
> + if (steal_refs)
> scm_release(siocb->scm);
> else
> scm_destroy(siocb->scm);
> @@ -1687,9 +1692,10 @@ pipe_err:
> send_sig(SIGPIPE, current, 0);
> err = -EPIPE;
> out_err:
> - if (skb == NULL)
> + if (steal_refs)
> + scm_release(siocb->scm);
> + else
> scm_destroy(siocb->scm);
> -out:
> siocb->scm = NULL;
> return sent ? : err;
> }
>
>
>
Signed-off-by: Zheng Yan <zheng.z.yan@intel.com>
^ permalink raw reply
* Re: [PATCH] per-cgroup tcp buffer limitation
From: Glauber Costa @ 2011-09-08 4:44 UTC (permalink / raw)
To: Greg Thelen
Cc: linux-kernel, linux-mm, containers, netdev, xemul,
David S. Miller, Hiroyouki Kamezawa, Eric W. Biederman,
Suleiman Souhlal
In-Reply-To: <CAHH2K0aq4s1_H-yY0kA3LhM00CCNNbJZyvyBoDD6rHC+qo_gNg@mail.gmail.com>
On 09/07/2011 06:35 PM, Greg Thelen wrote:
> On Tue, Sep 6, 2011 at 3:37 PM, Glauber Costa<glommer@parallels.com> wrote:
>> I think memcg's usage is really all you need here. In the end of the day, it
>> tells you how many pages your container has available. The whole
>> point of kmem cgroup is not any kind of reservation or accounting.
>
> The memcg does not reserve memory. It provides upper bound limits on
> memory usage. A careful admin can configure soft_limit_in_bytes as an
> approximation of a memory reservation. But the soft limit is really
> more like a reclaim target when there is global memory pressure.
>
>> Once a container (or cgroup) reaches a number of objects *pinned* in memory
>> (therefore, non-reclaimable), you won't be able to grab anything from it.
>>
>>> So
>>> far my use cases involve a single memory limit which includes both
>>> kernel and user memory. So I would need a user space agent to poll
>>> {memcg,kmem}.usage_in_bytes to apply pressure to memcg if kmem grows
>>> and visa versa.
>>
>> Maybe not.
>> If userspace memory works for you today (supposing it does), why change?
>
> Good question. Current upstream memcg user space memory limit does
> not work for me today. I should have made that more obvious (sorry).
> See below for details.
>
>> Right now you assign X bytes of user memory to a container, and the kernel
>> memory is shared among all of them. If this works for you, kmem_cgroup won't
>> change that. It just will impose limits over which
>> your kernel objects can't grow.
>>
>> So you don't *need* a userspace agent doing this calculation, because
>> fundamentally, nothing changed: I am not unbilling memory in memcg to bill
>> it back in kmem_cg. Of course, once it is in, you will be able to do it in
>> such a fine grained fashion if you decide to do so.
>>
>>> Do you foresee instantiation of multiple kmem cgroups, so that a
>>> process could be added into kmem/K1 or kmem/K2? If so do you plan on
>>> supporting migration between cgroups and/or migration of kmem charge
>>> between K1 to K2?
>>
>> Yes, each container should have its own cgroup, so at least in the use
>> cases I am concerned, we will have a lot of them. But the usual lifecycle,
>> is create, execute and die. Mobility between them
>> is not something I am overly concerned right now.
>>
>>
>>>>> Do you foresee the kmem cgroup growing to include reclaimable slab,
>>>>> where freeing one type of memory allows for reclaim of the other?
>>>>
>>>> Yes, absolutely.
>
> Now I see that you're using kmem to limit the amount of unreclaimable
> kernel memory.
>
> We have a work-in-progress patch series that adds kernel memory accounting to
> memcg. These patches allow an admin to specify a single memory limit
> for a cgroup which encompasses both user memory (as upstream memcg
> does) and also includes many kernel memory allocations (especially
> slab, page-tables). When kernel memory grows it puts pressure on user
> memory; when user memory grows it puts pressure on reclaimable kernel
> memory using registered shrinkers. We are in the process of cleaning
> up these memcg slab accounting patches.
>
> In my uses cases there is a single memory limit that applies to both
> kernel and user memory. If a separate kmem cgroup is introduced to
> manage kernel memory outside of memcg with a distinct limit, then I
> would need a user space daemon which balances memory between the kmem
> and memcg subsystems. As kmem grows, this daemon would apply pressure
> to memcg, and as memcg grows pressure would be applied to kmem. As
> you stated kernel memory is not necessarily reclaimable. So such
> reclaim may fail. My resistance to this approach is that with a
> single memory cgroup admins can do a better job packing a machine. If
> balancing daemons are employed then more memory would need to be
> reserved and more user space cpu time would be needed to apply VM
> pressure between the types of memory.
Well, it is a way to see this. The other way to see this, is that you're
proposing to move to the kernel, something that really belongs in
userspace. That's because:
With the information you provided me, I have no reason to believe that
the kernel has more condition to do this work. Do the kernel have access
to any information that userspace do not, and can't be exported? If not,
userspace is traditionally where this sort of stuff has been done.
Using userspace CPU is no different from using kernel cpu in this
particular case. It is all overhead, regardless where it comes from.
Moreover, you end up setting up a policy, instead of a mechanism. What
should be this proportion? Do we reclaim everything with the same
frequency? Should we be more tolerant with a specific container?
Also, If you want to allow any flexibility in this scheme, like: "Should
this network container be able to stress the network more, pinning more
memory, but not other subsystems?", you end up having to touch all
individual files anyway - probably with a userspace daemon.
Also, as you noticed yourself, kernel memory is fundamentally different
from userspace memory. You can't just set reclaim limits, since you have
no guarantees it will work. User memory is not a scarce resource.
Kernel memory is.
>
> While there are people (like me) who want a combined memory usage
> limit there are also people (like you) who want separate user and
> kernel limiting.
Combined excludes separate. Separate does not exclude combined.
> I have toyed with the idea of having a per cgroup
> flag that determines if kernel and user memory should be combined
> charged against a single limit or if they should have separate limits.
And then every other kind of mechanism one may think of involves a
kernel patch, instead of a potentially simple userspace change.
> I have also wondered if there was a way to wire the usage of two
> subsystems together, then it would also meet meet my needs. But I am
> not sure how to do that.
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v2 3/9] socket: initial cgroup code.
From: Glauber Costa @ 2011-09-08 4:54 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: linux-kernel, xemul, netdev, linux-mm, Eric W. Biederman,
containers, David S. Miller
In-Reply-To: <20110907221710.GA7845@shutemov.name>
On 09/07/2011 07:17 PM, Kirill A. Shutemov wrote:
> On Wed, Sep 07, 2011 at 01:23:13AM -0300, Glauber Costa wrote:
>> We aim to control the amount of kernel memory pinned at any
>> time by tcp sockets. To lay the foundations for this work,
>> this patch adds a pointer to the kmem_cgroup to the socket
>> structure.
>>
>> Signed-off-by: Glauber Costa<glommer@parallels.com>
>> CC: David S. Miller<davem@davemloft.net>
>> CC: Hiroyouki Kamezawa<kamezawa.hiroyu@jp.fujitsu.com>
>> CC: Eric W. Biederman<ebiederm@xmission.com>
>> ---
>> include/linux/kmem_cgroup.h | 29 +++++++++++++++++++++++++++++
>> include/net/sock.h | 2 ++
>> net/core/sock.c | 5 ++---
>> 3 files changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/kmem_cgroup.h b/include/linux/kmem_cgroup.h
>> index 0e4a74b..77076d8 100644
>> --- a/include/linux/kmem_cgroup.h
>> +++ b/include/linux/kmem_cgroup.h
>> @@ -49,5 +49,34 @@ static inline struct kmem_cgroup *kcg_from_task(struct task_struct *tsk)
>> return NULL;
>> }
>> #endif /* CONFIG_CGROUP_KMEM */
>> +
>> +#ifdef CONFIG_INET
>
> Will it break something if you define the helpers even if CONFIG_INET
> is not defined?
> It will be much cleaner. You can reuse ifdef CONFIG_CGROUP_KMEM in this
> case.
The helpers inside CONFIG_INET are needed for the network code,
regardless of kmem cgroup is defined or not, not the other way around.
So I could remove CONFIG_INET, but I can't possibly move it inside
CONFIG_CGROUP_KMEM. So this buy us nothing.
>> +#include<net/sock.h>
>> +static inline void sock_update_kmem_cgrp(struct sock *sk)
>> +{
>> +#ifdef CONFIG_CGROUP_KMEM
>> + sk->sk_cgrp = kcg_from_task(current);
>> +
>> + /*
>> + * We don't need to protect against anything task-related, because
>> + * we are basically stuck with the sock pointer that won't change,
>> + * even if the task that originated the socket changes cgroups.
>> + *
>> + * What we do have to guarantee, is that the chain leading us to
>> + * the top level won't change under our noses. Incrementing the
>> + * reference count via cgroup_exclude_rmdir guarantees that.
>> + */
>> + cgroup_exclude_rmdir(&sk->sk_cgrp->css);
>> +#endif
>> +}
>> +
>> +static inline void sock_release_kmem_cgrp(struct sock *sk)
>> +{
>> +#ifdef CONFIG_CGROUP_KMEM
>> + cgroup_release_and_wakeup_rmdir(&sk->sk_cgrp->css);
>> +#endif
>> +}
>> +
>> +#endif /* CONFIG_INET */
>> #endif /* _LINUX_KMEM_CGROUP_H */
>
>> @@ -2252,9 +2254,6 @@ void sk_common_release(struct sock *sk)
>> }
>> EXPORT_SYMBOL(sk_common_release);
>>
>> -static DEFINE_RWLOCK(proto_list_lock);
>> -static LIST_HEAD(proto_list);
>> -
>
> Wrong patch?
Yes, it is. Thanks for noticing.
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [net-next-2.6 PATCH 0/3 RFC] macvlan: MAC Address filtering support for passthru mode
From: Roopa Prabhu @ 2011-09-08 5:20 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, dragos.tatulea, arnd, dwang2, benve, kaber, sri, davem,
eric.dumazet, mchan, kvm
In-Reply-To: <20110907123435.GF9337@redhat.com>
On 9/7/11 5:34 AM, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Sep 06, 2011 at 03:35:40PM -0700, Roopa Prabhu wrote:
>> This patch is an attempt at providing address filtering support for macvtap
>> devices in PASSTHRU mode. Its still a work in progress.
>> Briefly tested for basic functionality. Wanted to get some feedback on the
>> direction before proceeding.
>>
>
> Good work, thanks.
>
Thanks.
>> I have hopefully CC'ed all concerned people.
>
> kvm crowd might also be interested.
> Try using ./scripts/get_maintainer.pl as well.
>
Thanks for the tip. Expanded CC list a bit more.
>> PASSTHRU mode today sets the lowerdev in promiscous mode. In PASSTHRU mode
>> there is a 1-1 mapping between macvtap device and physical nic or VF. And all
>> filtering is done in lowerdev hw. The lowerdev does not need to be in
>> promiscous mode as long as the guest filters are passed down to the lowerdev.
>> This patch tries to remove the need for putting the lowerdev in promiscous
>> mode.
>> I have also referred to the thread below where TUNSETTXFILTER was mentioned
>> in
>> this context:
>> http://patchwork.ozlabs.org/patch/69297/
>>
>> This patch basically passes the addresses got by TUNSETTXFILTER to macvlan
>> lowerdev.
>>
>> I have looked at previous work and discussions on this for qemu-kvm
>> by Michael Tsirkin, Alex Williamson and Dragos Tatulea
>> http://patchwork.ozlabs.org/patch/78595/
>> http://patchwork.ozlabs.org/patch/47160/
>> https://patchwork.kernel.org/patch/474481/
>>
>> Redhat bugzilla by Michael Tsirkin:
>> https://bugzilla.redhat.com/show_bug.cgi?id=655013
>>
>> I used Michael's qemu-kvm patch for testing the changes with KVM
>>
>> I would like to cover both MAC and vlan filtering in this work.
>>
>> Open Questions/Issues:
>> - There is a need for vlan filtering to complete the patch. It will require
>> a new tap ioctl cmd for vlans.
>> Some ideas on this are:
>>
>> a) TUNSETVLANFILTER: This will entail we send the whole vlan bitmap filter
>> (similar to tun_filter for addresses). Passing the vlan id's to lower
>> device will mean going thru the whole list of vlans every time.
>>
>> OR
>>
>> b) TUNSETVLAN with vlan id and flag to set/unset
>>
>> Does option 'b' sound ok ?
>>
>> - In this implementation we make the macvlan address list same as the address
>> list that came in the filter with TUNSETTXFILTER. This will not cover cases
>> where the macvlan device needs to have other addresses that are not
>> necessarily in the filter. Is this a problem ?
>
> What cases do you have in mind?
>
This patch targets only macvlan PASSTHRU mode and for PASSTHRU mode I don't
see a problem with uc/mc address list being the same in all the stacked
netdevs in the path. I called that out above to make sure I was not missing
any case in PASSTHRU mode where this might be invalid. Otherwise I don't see
a problem in the simple PASSTHRU use case this patch supports.
>> - The patch currently only supports passing of IFF_PROMISC and IFF_MULTICAST
>> filter flags to lowerdev
>>
>> This patch series implements the following
>> 01/3 - macvlan: Add support for unicast filtering in macvlan
>> 02/3 - macvlan: Add function to set addr filter on lower device in passthru
>> mode
>> 03/3 - macvtap: Add support for TUNSETTXFILTER
>>
>> Please comment. Thanks.
>>
>> Signed-off-by: Roopa Prabhu <roprabhu@cisco.com>
>> Signed-off-by: Christian Benvenuti <benve@cisco.com>
>> Signed-off-by: David Wang <dwang2@cisco.com>
>
> The security isn't lower than with promisc, so I don't see
> a problem with this as such.
>
> There are more features we'll want down the road though,
> so let's see whether the interface will be able to
> satisfy them in a backwards compatible way before we
> set it in stone. Here's what I came up with:
>
> How will the filtering table be partitioned within guests?
Since this patch supports macvlan PASSTHRU mode only, in which the lower
device has 1-1 mapping to the guest nic, it does not require any
partitioning of filtering table within guests. Unless I missed understanding
something.
If the lower device were being shared by multiple guest network interfaces
(non PASSTHRU mode), only then we will need to maintain separate filter
tables for each guest network interface in macvlan and forward the pkt to
respective guest interface after a filter lookup. This could affect
performance too I think.
I chose to support PASSTHRU Mode only at first because its simpler and all
code additions are in control path only.
>
> A way to limit what the guest can do would also be useful.
> How can this be done? selinux?
I vaguely remember a thread on the same context.. had a suggestion to
maintain pre-approved address lists and allow guest filter registration of
only those addresses for security. This seemed reasonable. Plus the ability
to support additional address registration from guest could be made
configurable (One of your ideas again from prior work).
I am not an selinux expert, but I am thinking we can use it to only allow or
disallow access or operations to the macvtap device. (?). I will check more
on this.
>
> Any thoughts on spoofing filtering?
I can only think of checking addresses against an allowed address list.
Don't know of any other ways. Any hints ?
In any case I am assuming all the protection/security measures should be
taken at the layer calling the TUNSETTXFILTER ie..In macvtap virtualization
use case its libvirt or qemu-kvm. No ?
>
> Would it be possible to make the filtering programmable
> using netlink, e.g. ethtool, ip, or some such?
Should be possible via ethtool or ip calling ioctl TUNSETTXFILTER. Are you
thinking of macvlan having a netlink interface to set filter and not ioctl
?. Sure. But I was thinking the point of implementing TUNSETTXFILTER was to
maintain compatibility with the generic tap interface that does the same
thing.
And having both the netlink op and ioctl interface might not be clean ?.
Sorry if I misunderstood your question.
> That would make this useful for bridged setups besides
> macvtap/virtualization.
>
Thanks for the comments.
^ permalink raw reply
* Re: [PATCH v2 3/9] socket: initial cgroup code.
From: Kirill A. Shutemov @ 2011-09-08 5:35 UTC (permalink / raw)
To: Glauber Costa
Cc: linux-kernel, xemul, netdev, linux-mm, Eric W. Biederman,
containers, David S. Miller
In-Reply-To: <4E684A6B.6030205@parallels.com>
On Thu, Sep 08, 2011 at 01:54:03AM -0300, Glauber Costa wrote:
> On 09/07/2011 07:17 PM, Kirill A. Shutemov wrote:
> > On Wed, Sep 07, 2011 at 01:23:13AM -0300, Glauber Costa wrote:
> >> We aim to control the amount of kernel memory pinned at any
> >> time by tcp sockets. To lay the foundations for this work,
> >> this patch adds a pointer to the kmem_cgroup to the socket
> >> structure.
> >>
> >> Signed-off-by: Glauber Costa<glommer@parallels.com>
> >> CC: David S. Miller<davem@davemloft.net>
> >> CC: Hiroyouki Kamezawa<kamezawa.hiroyu@jp.fujitsu.com>
> >> CC: Eric W. Biederman<ebiederm@xmission.com>
> >> ---
> >> include/linux/kmem_cgroup.h | 29 +++++++++++++++++++++++++++++
> >> include/net/sock.h | 2 ++
> >> net/core/sock.c | 5 ++---
> >> 3 files changed, 33 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/linux/kmem_cgroup.h b/include/linux/kmem_cgroup.h
> >> index 0e4a74b..77076d8 100644
> >> --- a/include/linux/kmem_cgroup.h
> >> +++ b/include/linux/kmem_cgroup.h
> >> @@ -49,5 +49,34 @@ static inline struct kmem_cgroup *kcg_from_task(struct task_struct *tsk)
> >> return NULL;
> >> }
> >> #endif /* CONFIG_CGROUP_KMEM */
> >> +
> >> +#ifdef CONFIG_INET
> >
> > Will it break something if you define the helpers even if CONFIG_INET
> > is not defined?
> > It will be much cleaner. You can reuse ifdef CONFIG_CGROUP_KMEM in this
> > case.
>
> The helpers inside CONFIG_INET are needed for the network code,
> regardless of kmem cgroup is defined or not, not the other way around.
>
> So I could remove CONFIG_INET, but I can't possibly move it inside
> CONFIG_CGROUP_KMEM. So this buy us nothing.
You can define empty under CONFIG_CGROUP_KMEM's #else, can't you?
Like with kcg_from_cgroup()/kcg_from_task().
--
Kirill A. Shutemov
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH -next v2] unix stream: Fix use-after-free crashes
From: Eric Dumazet @ 2011-09-08 5:59 UTC (permalink / raw)
To: Tim Chen
Cc: Yan, Zheng, sedat.dilek@gmail.com, Yan, Zheng,
netdev@vger.kernel.org, davem@davemloft.net, sfr@canb.auug.org.au,
jirislaby@gmail.com, Shi, Alex, Valdis Kletnieks
In-Reply-To: <1315429583.2361.3.camel@schen9-mobl>
Le mercredi 07 septembre 2011 à 14:06 -0700, Tim Chen a écrit :
> On Thu, 2011-09-08 at 08:27 +0800, Yan, Zheng wrote:
>
> > > err = -EPIPE;
> > > out_err:
> > > - if (skb == NULL)
> > > + if (!steal_refs)
> > > scm_destroy(siocb->scm);
> >
> > I think we should call scm_release() here in the case of
> > steal_refs == true. Otherwise siocb->scm->fp may leak.
>
> Yan Zheng,
>
> I've updated the patch. If it looks good to you now, can you add your
> Signed-off-by to the patch. Pending Sedat's testing on this patch,
> I think it is good to go.
>
> Tim
>
> ---
> Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
> in Unix socket's send and receive path) introduced a use-after-free bug.
> The sent skbs from unix_stream_sendmsg could be consumed and destructed
> by the receive side, removing all references to the credentials,
> before the send side has finished sending out all
> packets. However, send side could continue to consturct new packets in the
> stream, using credentials that have lost its last reference and been
> freed.
>
> In this fix, we don't steal the reference to credentials we have obtained
> in scm_send at beginning of unix_stream_sendmsg, till we've reached
> the last packet. This fixes the problem in commit 0856a30409.
>
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> Reported-by: Jiri Slaby <jirislaby@gmail.com>
> Tested-by: Sedat Dilek <sedat.dilek@googlemail.com>
> Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
> ---
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 136298c..47780dc 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1383,10 +1383,11 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
> }
>
> static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb,
> - bool send_fds, bool ref)
> + bool send_fds, bool steal_refs)
> {
> int err = 0;
> - if (ref) {
> +
> + if (!steal_refs) {
> UNIXCB(skb).pid = get_pid(scm->pid);
> UNIXCB(skb).cred = get_cred(scm->cred);
> } else {
> @@ -1458,7 +1459,7 @@ static int unix_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock,
> if (skb == NULL)
> goto out;
>
> - err = unix_scm_to_skb(siocb->scm, skb, true, false);
> + err = unix_scm_to_skb(siocb->scm, skb, true, true);
> if (err < 0)
> goto out_free;
> max_level = err + 1;
> @@ -1581,6 +1582,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
> int sent = 0;
> struct scm_cookie tmp_scm;
> bool fds_sent = false;
> + bool steal_refs = false;
> int max_level;
>
> if (NULL == siocb->scm)
> @@ -1642,11 +1644,14 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
> size = min_t(int, size, skb_tailroom(skb));
>
>
> - /* Only send the fds and no ref to pid in the first buffer */
> - err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
> + /* Only send the fds in first buffer
> + * Last buffer can steal our references to pid/cred
> + */
> + steal_refs = (sent + size >= len);
> + err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, steal_refs);
> if (err < 0) {
> kfree_skb(skb);
> - goto out;
> + goto out_err;
> }
> max_level = err + 1;
> fds_sent = true;
> @@ -1654,7 +1659,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
> err = memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size);
> if (err) {
> kfree_skb(skb);
> - goto out;
> + goto out_err;
> }
>
> unix_state_lock(other);
> @@ -1671,7 +1676,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
> sent += size;
> }
>
> - if (skb)
> + if (steal_refs)
> scm_release(siocb->scm);
> else
> scm_destroy(siocb->scm);
> @@ -1687,9 +1692,10 @@ pipe_err:
> send_sig(SIGPIPE, current, 0);
> err = -EPIPE;
> out_err:
> - if (skb == NULL)
> + if (steal_refs)
> + scm_release(siocb->scm);
> + else
> scm_destroy(siocb->scm);
> -out:
> siocb->scm = NULL;
> return sent ? : err;
> }
>
>
>
I dont think this patch is good.
Sedat traces have nothing to do with af_unix.
Once unix_scm_to_skb() was called and successful, and steal_refs is true
our refs are attached to this skb. They will be released by
skb_free(skb). Same for fp : They either were sent in a previous skb or
this one.
This is why the "goto out;" was OK.
^ permalink raw reply
* Re: [RFC] interface for outgoing packet
From: Eric Dumazet @ 2011-09-08 6:09 UTC (permalink / raw)
To: Viral Mehta; +Cc: netdev
In-Reply-To: <CANX6DanC8DJsAZj4dV3oZo3DnLV6AyxBDm82vHaJm3C51aRqEg@mail.gmail.com>
Le mercredi 07 septembre 2011 à 20:24 -0400, Viral Mehta a écrit :
> Hi All,
>
> How reasonably I can assume that,
>
> on whatever interface, I received the last packet
> for a particular Socket Connection, the same interface will be
> used to SEND the next packet for same socket connection?
>
No
> I am collecting the logs on one of my test machines.
> And it shows, I can assume all the time that "interface" which received packet
> for some socket connection, the same will be used to send packet for
> that connection
>
> But, I am not sure if I am missing some scenarios
> or setup (for e.g., bonding) where it can be wrong ?
>
> It would be more help if some one can shed some light.
By default, a socket is not bound to one interface, so you cant assume
this.
Check SO_BINDTODEVICE socket option if you need it.
^ permalink raw reply
* Re: [PATCH -next v2] unix stream: Fix use-after-free crashes
From: Eric Dumazet @ 2011-09-08 6:21 UTC (permalink / raw)
To: Tim Chen
Cc: davem@davemloft.net, sedat.dilek@gmail.com, Yan, Zheng,
netdev@vger.kernel.org, sfr@canb.auug.org.au, jirislaby@gmail.com,
Shi, Alex, Valdis Kletnieks, Yan, Zheng
In-Reply-To: <1315430115.2361.11.camel@schen9-mobl>
Le mercredi 07 septembre 2011 à 14:15 -0700, Tim Chen a écrit :
> Oops, I've forgotten to add Eric's previous Signed-off-by in my latest
> patch log. David, please add that when you pick up the patch.
> Once Yan Zheng added his sign off and Sedat tested the patch, I think it
> will be good to go.
Tim, as soon as you post another patch version, you must remove all
prior Signed-off-by, and only add yours.
So it was fine to do so.
By the way your last version introduce a new bug, so I would rather NACK
it ;)
^ permalink raw reply
* Re: [PATCH -next v2] unix stream: Fix use-after-free crashes
From: Yan, Zheng @ 2011-09-08 6:22 UTC (permalink / raw)
To: Eric Dumazet
Cc: Tim Chen, sedat.dilek@gmail.com, Yan, Zheng,
netdev@vger.kernel.org, davem@davemloft.net, sfr@canb.auug.org.au,
jirislaby@gmail.com, Shi, Alex, Valdis Kletnieks
In-Reply-To: <1315461572.2532.7.camel@edumazet-laptop>
On 09/08/2011 01:59 PM, Eric Dumazet wrote:
> Le mercredi 07 septembre 2011 à 14:06 -0700, Tim Chen a écrit :
>> On Thu, 2011-09-08 at 08:27 +0800, Yan, Zheng wrote:
>>
>>>> err = -EPIPE;
>>>> out_err:
>>>> - if (skb == NULL)
>>>> + if (!steal_refs)
>>>> scm_destroy(siocb->scm);
>>>
>>> I think we should call scm_release() here in the case of
>>> steal_refs == true. Otherwise siocb->scm->fp may leak.
>>
>> Yan Zheng,
>>
>> I've updated the patch. If it looks good to you now, can you add your
>> Signed-off-by to the patch. Pending Sedat's testing on this patch,
>> I think it is good to go.
>>
>> Tim
>>
>> ---
>> Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
>> in Unix socket's send and receive path) introduced a use-after-free bug.
>> The sent skbs from unix_stream_sendmsg could be consumed and destructed
>> by the receive side, removing all references to the credentials,
>> before the send side has finished sending out all
>> packets. However, send side could continue to consturct new packets in the
>> stream, using credentials that have lost its last reference and been
>> freed.
>>
>> In this fix, we don't steal the reference to credentials we have obtained
>> in scm_send at beginning of unix_stream_sendmsg, till we've reached
>> the last packet. This fixes the problem in commit 0856a30409.
>>
>> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
>> Reported-by: Jiri Slaby <jirislaby@gmail.com>
>> Tested-by: Sedat Dilek <sedat.dilek@googlemail.com>
>> Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
>> ---
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index 136298c..47780dc 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -1383,10 +1383,11 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
>> }
>>
>> static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb,
>> - bool send_fds, bool ref)
>> + bool send_fds, bool steal_refs)
>> {
>> int err = 0;
>> - if (ref) {
>> +
>> + if (!steal_refs) {
>> UNIXCB(skb).pid = get_pid(scm->pid);
>> UNIXCB(skb).cred = get_cred(scm->cred);
>> } else {
>> @@ -1458,7 +1459,7 @@ static int unix_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock,
>> if (skb == NULL)
>> goto out;
>>
>> - err = unix_scm_to_skb(siocb->scm, skb, true, false);
>> + err = unix_scm_to_skb(siocb->scm, skb, true, true);
>> if (err < 0)
>> goto out_free;
>> max_level = err + 1;
>> @@ -1581,6 +1582,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
>> int sent = 0;
>> struct scm_cookie tmp_scm;
>> bool fds_sent = false;
>> + bool steal_refs = false;
>> int max_level;
>>
>> if (NULL == siocb->scm)
>> @@ -1642,11 +1644,14 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
>> size = min_t(int, size, skb_tailroom(skb));
>>
>>
>> - /* Only send the fds and no ref to pid in the first buffer */
>> - err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
>> + /* Only send the fds in first buffer
>> + * Last buffer can steal our references to pid/cred
>> + */
>> + steal_refs = (sent + size >= len);
>> + err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, steal_refs);
>> if (err < 0) {
>> kfree_skb(skb);
>> - goto out;
>> + goto out_err;
>> }
>> max_level = err + 1;
>> fds_sent = true;
>> @@ -1654,7 +1659,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
>> err = memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size);
>> if (err) {
>> kfree_skb(skb);
>> - goto out;
>> + goto out_err;
>> }
>>
>> unix_state_lock(other);
>> @@ -1671,7 +1676,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
>> sent += size;
>> }
>>
>> - if (skb)
>> + if (steal_refs)
>> scm_release(siocb->scm);
>> else
>> scm_destroy(siocb->scm);
>> @@ -1687,9 +1692,10 @@ pipe_err:
>> send_sig(SIGPIPE, current, 0);
>> err = -EPIPE;
>> out_err:
>> - if (skb == NULL)
>> + if (steal_refs)
>> + scm_release(siocb->scm);
>> + else
>> scm_destroy(siocb->scm);
>> -out:
>> siocb->scm = NULL;
>> return sent ? : err;
>> }
>>
>>
>>
>
> I dont think this patch is good.
>
> Sedat traces have nothing to do with af_unix.
>
> Once unix_scm_to_skb() was called and successful, and steal_refs is true
> our refs are attached to this skb. They will be released by
> skb_free(skb). Same for fp : They either were sent in a previous skb or
> this one.
>
> This is why the "goto out;" was OK.
>
I don't think so. unix_scm_to_skb() calls unix_attach_fds(), it
always duplicates scm->fp.
Regards
^ permalink raw reply
* [PATCH] net: phy: Add config option to specify external switch port to be used if switch is used as PHY
From: Lambrecht Jürgen @ 2011-09-08 6:54 UTC (permalink / raw)
To: netdev@vger.kernel.org; +Cc: linux-embedded@vger.kernel.org
Hello,
In our embedded designs, this is a useful patch. Maybe it can be useful
for somebody else too.
Or maybe there are already better solutions?
I know I could also write a driver for our switch, but that is too much
effort just to select the active port.
Kind regards,
Jürgen
In embedded design, instead of a PHY, sometimes a switch is used that
behaves as a PHY through its MII port. For example to use a daisy
chain network configuration instead of an expensive star config.
In that case, many phy ports are available, but only 1 should
be used
to check link status, and not the first one available as is
the case
without this configuration (that is, set to its default value 0).
So this options specifies the switch port number to be used
to check
link status, because if the link is down, no data is sent by the
TCP/IP stack.
Signed-off-by: Jürgen Lambrecht <J.Lambrecht@televic.com>
---
drivers/net/phy/Kconfig | 14 ++++++++++++++
drivers/net/phy/mdio_bus.c | 9 +++++++++
2 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index a702443..554561f 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -13,6 +13,20 @@ menuconfig PHYLIB
if PHYLIB
+config SWITCH_PHY
+ int "External switch port to be used if switch is used as PHY"
+ default "0"
+ help
+ In embedded design, instead of a PHY, sometimes a switch is
used that
+ behaves as a PHY through its MII port. For example to use a daisy
+ chain network configuration instead of an expensive star config.
+ In that case, many phy ports are available, but only 1 should
be used
+ to check link status, and not the first one available as is
the case
+ without this configuration (that is, set to its default value 0).
+ So this options specifies the switch port number to be used to
check
+ link status, because if the link is down, no data is sent by the
+ TCP/IP stack.
+
comment "MII PHY device drivers"
config MARVELL_PHY
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 6c58da2..016437a 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -112,7 +112,14 @@ int mdiobus_register(struct mii_bus *bus)
if (bus->reset)
bus->reset(bus);
+ /* The config below is always availble with CONFIG_PHYLIB. If 0, the
+ behavior is as before without this patch (or P0 of the switch is
+ taken because it is the first one found). */
+#if CONFIG_SWITCH_PHY
+ i = CONFIG_SWITCH_PHY;
+#else
for (i = 0; i < PHY_MAX_ADDR; i++) {
+#endif
if ((bus->phy_mask & (1 << i)) == 0) {
struct phy_device *phydev;
@@ -122,7 +129,9 @@ int mdiobus_register(struct mii_bus *bus)
goto error;
}
}
+#if !CONFIG_SWITCH_PHY
}
+#endif
bus->state = MDIOBUS_REGISTERED;
pr_info("%s: probed\n", bus->name);
--
1.7.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox