* Re: use-after-free in usbnet
From: Huajun Li @ 2012-04-21 7:50 UTC (permalink / raw)
To: Ming Lei
Cc: Oliver Neukum, Alan Stern, Dave Jones, netdev, linux-usb,
Fedora Kernel Team
In-Reply-To: <CACVXFVN6kvXk7s4Sc0d_-yKSM=rV3qcuPPBHVZYzoQjnwkGX+Q@mail.gmail.com>
On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei <tom.leiming@gmail.com> wrote:
> Hi Huajun,
>
> On Sat, Apr 21, 2012 at 2:39 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>
>> Hi Ming,
>> That's why my patch uses skb_queue_walk() to traverse the queue,
>> it guarantees the skb available in each loop. Is this what you
>> expected?
>>
>> The main idea of my patch(it is based on current mainline: 3.4.0-rc3) is:
>> 1. If the skb in txq/rxq, then it must be available,
>> unlink_urbs() can refer to it safely while it holds q->lock;
>> 2. If the skb in txq/rxq and its state is
>> rx_done/tx_done/rx_cleanup, that means the skb's URB is complete, then
>> don't need to unlink it again;
>
> As I said before, at least current code is not mistaken, since unlink
> can handle the case correctly.
>
>> 3. Before releasing q->lock in unlink_urbs(), it will increase
>> the URB's refercount, so even the related skb is freed in future, the
>> URB is still available.
>
> No, increasing URB's reference count does not prevent the referenced skb
> from being freed, see usbnet_bh(), so 'tmp = skb->next' in skb_queue_walk_safe
> still may reference a freed pointer.
>
Did we on the same page, could you please review my patch again?
My draft patch was based on current mainline( 3.4.0-rc3) which had
already integrated your previous patch. And in my patch, it replaced
skb_queue_walk_safe() with skb_queue_walk(), so you will not see 'tmp
= skb->next' any more.
>>> As far as I can think of, we can hold lock of done queue to forbid skb free
>>> during unlinking. The below patch may fix the problem, are you OK
>>> with it?
>>
>> Just skip trying this per your following email's comments.
>
> I will prepare a new patch later, if you'd like to try it.
>
>
> Thanks
> --
> Ming Lei
^ permalink raw reply
* Re: use-after-free in usbnet
From: Ming Lei @ 2012-04-21 7:45 UTC (permalink / raw)
To: Huajun Li
Cc: Oliver Neukum, Alan Stern, Dave Jones,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Fedora Kernel Team
In-Reply-To: <CACVXFVN6kvXk7s4Sc0d_-yKSM=rV3qcuPPBHVZYzoQjnwkGX+Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Huajun,
On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Just skip trying this per your following email's comments.
>
> I will prepare a new patch later, if you'd like to try it.
The below patch reverts the below commits:
0956a8c20b23d429e79ff86d4325583fc06f9eb4
(usbnet: increase URB reference count before usb_unlink_urb)
4231d47e6fe69f061f96c98c30eaf9fb4c14b96d
(net/usbnet: avoid recursive locking in usbnet_stop())
and keep holding tx/rx queue lock during unlinking, but avoid
to acquire the same queue lock inside complete handler triggered by
usb_unlink_urb.
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index db99536..effb34e 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -291,9 +291,11 @@ static void defer_bh(struct usbnet *dev, struct
sk_buff *skb, struct sk_buff_hea
{
unsigned long flags;
- spin_lock_irqsave(&list->lock, flags);
+ if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
+ spin_lock_irqsave(&list->lock, flags);
__skb_unlink(skb, list);
- spin_unlock(&list->lock);
+ if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
+ spin_unlock(&list->lock);
spin_lock(&dev->done.lock);
__skb_queue_tail(&dev->done, skb);
if (dev->done.qlen == 1)
@@ -345,7 +347,8 @@ static int rx_submit (struct usbnet *dev, struct
urb *urb, gfp_t flags)
usb_fill_bulk_urb (urb, dev->udev, dev->in,
skb->data, size, rx_complete, skb);
- spin_lock_irqsave (&dev->rxq.lock, lockflags);
+ if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
+ spin_lock_irqsave (&dev->rxq.lock, lockflags);
if (netif_running (dev->net) &&
netif_device_present (dev->net) &&
@@ -377,7 +380,8 @@ static int rx_submit (struct usbnet *dev, struct
urb *urb, gfp_t flags)
netif_dbg(dev, ifdown, dev->net, "rx: stopped\n");
retval = -ENOLINK;
}
- spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
+ if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
+ spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
if (retval) {
dev_kfree_skb_any (skb);
usb_free_urb (urb);
@@ -582,6 +586,7 @@ static int unlink_urbs (struct usbnet *dev, struct
sk_buff_head *q)
int count = 0;
spin_lock_irqsave (&q->lock, flags);
+ set_cpu_bit(URB_UNLINKING, dev->cpuflags);
skb_queue_walk_safe(q, skb, skbnext) {
struct skb_data *entry;
struct urb *urb;
@@ -590,15 +595,6 @@ static int unlink_urbs (struct usbnet *dev,
struct sk_buff_head *q)
entry = (struct skb_data *) skb->cb;
urb = entry->urb;
- /*
- * Get reference count of the URB to avoid it to be
- * freed during usb_unlink_urb, which may trigger
- * use-after-free problem inside usb_unlink_urb since
- * usb_unlink_urb is always racing with .complete
- * handler(include defer_bh).
- */
- usb_get_urb(urb);
- spin_unlock_irqrestore(&q->lock, flags);
// during some PM-driven resume scenarios,
// these (async) unlinks complete immediately
retval = usb_unlink_urb (urb);
@@ -606,9 +602,8 @@ static int unlink_urbs (struct usbnet *dev, struct
sk_buff_head *q)
netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
else
count++;
- usb_put_urb(urb);
- spin_lock_irqsave(&q->lock, flags);
}
+ clear_cpu_bit(URB_UNLINKING, dev->cpuflags);
spin_unlock_irqrestore (&q->lock, flags);
return count;
}
@@ -1283,6 +1278,7 @@ void usbnet_disconnect (struct usb_interface *intf)
usb_kill_urb(dev->interrupt);
usb_free_urb(dev->interrupt);
+ free_percpu(dev->cpuflags);
free_netdev(net);
usb_put_dev (xdev);
}
@@ -1353,6 +1349,13 @@ usbnet_probe (struct usb_interface *udev, const
struct usb_device_id *prod)
SET_NETDEV_DEV(net, &udev->dev);
dev = netdev_priv(net);
+
+ dev->cpuflags = alloc_percpu(unsigned long);
+ if (!dev->cpuflags) {
+ status = -ENOMEM;
+ goto out1;
+ }
+
dev->udev = xdev;
dev->intf = udev;
dev->driver_info = info;
@@ -1396,7 +1399,7 @@ usbnet_probe (struct usb_interface *udev, const
struct usb_device_id *prod)
if (info->bind) {
status = info->bind (dev, udev);
if (status < 0)
- goto out1;
+ goto out2;
// heuristic: "usb%d" for links we know are two-host,
// else "eth%d" when there's reasonable doubt. userspace
@@ -1465,6 +1468,8 @@ usbnet_probe (struct usb_interface *udev, const
struct usb_device_id *prod)
out3:
if (info->unbind)
info->unbind (dev, udev);
+out2:
+ free_percpu(dev->cpuflags);
out1:
free_netdev(net);
out:
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 605b0aa..2dc46f5 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -69,8 +69,28 @@ struct usbnet {
# define EVENT_DEV_WAKING 6
# define EVENT_DEV_ASLEEP 7
# define EVENT_DEV_OPEN 8
+ unsigned long __percpu *cpuflags;
+# define URB_UNLINKING 0
};
+static inline void set_cpu_bit(int nr, unsigned long __percpu *addr)
+{
+ unsigned long *fl = __this_cpu_ptr(addr);
+ set_bit(nr, fl);
+}
+
+static inline void clear_cpu_bit(int nr, unsigned long __percpu *addr)
+{
+ unsigned long *fl = __this_cpu_ptr(addr);
+ clear_bit(nr, fl);
+}
+
+static inline int test_cpu_bit(int nr, unsigned long __percpu *addr)
+{
+ unsigned long *fl = __this_cpu_ptr(addr);
+ return test_bit(nr, fl);
+}
+
static inline struct usb_driver *driver_of(struct usb_interface *intf)
{
return to_usb_driver(intf->dev.driver);
Thanks,
--
Ming Lei
--
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 related
* Re: use-after-free in usbnet
From: Ming Lei @ 2012-04-21 7:06 UTC (permalink / raw)
To: Huajun Li
Cc: Oliver Neukum, Alan Stern, Dave Jones,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Fedora Kernel Team
In-Reply-To: <CA+v9cxZZpL5soSga=MX_bD45KNve-Lnr2Qb6+gr7Mv6Txyh-fA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Huajun,
On Sat, Apr 21, 2012 at 2:39 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Ming,
> That's why my patch uses skb_queue_walk() to traverse the queue,
> it guarantees the skb available in each loop. Is this what you
> expected?
>
> The main idea of my patch(it is based on current mainline: 3.4.0-rc3) is:
> 1. If the skb in txq/rxq, then it must be available,
> unlink_urbs() can refer to it safely while it holds q->lock;
> 2. If the skb in txq/rxq and its state is
> rx_done/tx_done/rx_cleanup, that means the skb's URB is complete, then
> don't need to unlink it again;
As I said before, at least current code is not mistaken, since unlink
can handle the case correctly.
> 3. Before releasing q->lock in unlink_urbs(), it will increase
> the URB's refercount, so even the related skb is freed in future, the
> URB is still available.
No, increasing URB's reference count does not prevent the referenced skb
from being freed, see usbnet_bh(), so 'tmp = skb->next' in skb_queue_walk_safe
still may reference a freed pointer.
>> As far as I can think of, we can hold lock of done queue to forbid skb free
>> during unlinking. The below patch may fix the problem, are you OK
>> with it?
>
> Just skip trying this per your following email's comments.
I will prepare a new patch later, if you'd like to try it.
Thanks
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/3] don't take cgroup_mutex in destroy()
From: Li Zefan @ 2012-04-21 6:47 UTC (permalink / raw)
To: Glauber Costa
Cc: Tejun Heo, netdev-u79uwXL29TY76Z2rM5mHXA,
cgroups-u79uwXL29TY76Z2rM5mHXA,
kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, David Miller,
devel-GEFAQzZX7r8dnm+yROfE0A, Vivek Goyal
In-Reply-To: <4F917AEB.7080404-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Glauber Costa wrote:
> On 04/19/2012 07:57 PM, Tejun Heo wrote:
>> On Thu, Apr 19, 2012 at 07:49:17PM -0300, Glauber Costa wrote:
>>> Most of the destroy functions are only doing very simple things
>>> like freeing memory.
>>>
>>> The ones who goes through lists and such, already use its own
>>> locking for those.
>>>
>>> * The cgroup itself won't go away until we free it, (after destroy)
>>> * The parent won't go away because we hold a reference count
>>> * There are no more tasks in the cgroup, and the cgroup is declared
>>> dead (cgroup_is_removed() == true)
>>>
>>> For the blk-cgroup and the cpusets, I got the impression that the mutex
>>> is still necessary.
>>>
>>> For those, I grabbed it from within the destroy function itself.
>>>
>>> If the maintainer for those subsystems consider it safe to remove
>>> it, we can discuss it separately.
>>
>> I really don't like cgroup_lock() usage spreading more. It's
>> something which should be contained in cgroup.c proper. I looked at
>> the existing users a while ago and they seemed to be compensating
>> deficencies in API, so, if at all possible, let's not spread the
>> disease.
>
> Well, I can dig deeper and see if they are really needed. I don't know cpusets and blkcg *that* well, that's why I took them there, hoping that someone could enlighten me, maybe they aren't really needed even now.
>
> I agree with the compensating: As I mentioned, most of them are already taking other kinds of lock to protect their structures, which is the right thing to do.
>
> There were only two or three spots in cpusets and blkcg where I wasn't that sure that we could drop the lock... What do you say about that ?
> .
We can drop cgroup_mutex for cpusets with changes like this:
(Note: as I'm not able to get the latest code at this momment, this patch is based on 3.0.)
There are several places reading number_of_cpusets, but no one holds cgroup_mutex, except
the one in generate_sched_domains(). With this patch, both cpuset_create() and
generate_sched_domains() are still holding cgroup_mutex, so it's safe.
--- linux-kernel/kernel/cpuset.c.orig 2012-04-21 01:55:57.000000000 -0400
+++ linux-kernel/kernel/cpuset.c 2012-04-21 02:30:53.000000000 -0400
@@ -1876,7 +1876,9 @@ static struct cgroup_subsys_state *cpuse
cs->relax_domain_level = -1;
cs->parent = parent;
+ mutex_lock(&callback_mutex);
number_of_cpusets++;
+ mutex_unlock(&callback_mutex);
return &cs->css ;
}
@@ -1890,10 +1892,18 @@ static void cpuset_destroy(struct cgroup
{
struct cpuset *cs = cgroup_cs(cont);
- if (is_sched_load_balance(cs))
+ if (is_sched_load_balance(cs)) {
+ /*
+ * This cpuset is under destruction, so no one else can
+ * modify it, so it's safe to call update_flag() without
+ * cgroup_lock.
+ */
update_flag(CS_SCHED_LOAD_BALANCE, cs, 0);
+ }
+ mutex_lock(&callback_mutex);
number_of_cpusets--;
+ mutex_lock(&callback_mutex);
free_cpumask_var(cs->cpus_allowed);
kfree(cs);
}
^ permalink raw reply
* <FYI>
From: Alan Harrison @ 2012-04-03 3:22 UTC (permalink / raw)
My name is Alan Harrison, Attorney at law. Sequel to your non response to my previous email, I am re-sending this to you again thus; A deceased client of mine, that shares the same last name as yours,who died as the result of heart-related condition on January 28th 2009. I have contacted you to assist in distributing the money left behind by my client which is lodged in the bank.
Regards,
Alan Harrison
^ permalink raw reply
* Re: use-after-free in usbnet
From: Huajun Li @ 2012-04-21 6:39 UTC (permalink / raw)
To: Ming Lei
Cc: Oliver Neukum, Alan Stern, Dave Jones,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Fedora Kernel Team
In-Reply-To: <CACVXFVOc0XZ+eLHGiVwKuiUResRk8Cj9MS4EPMx7k57a0tEJhA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Sat, Apr 21, 2012 at 9:49 AM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>
>>>> Above patch has already been integrated to mainline. However, maybe
>>>> there still exists another potentail use-after-free issue, here is a
>>>> case:
>>>> After release the lock in unlink_urbs(), defer_bh() may move
>>>> current skb from rxq/txq to dev->done queue, even cause the skb be
>>>> released. Then in next loop cycle, it can't refer to expected skb, and
>>>> may Oops again.
>>>
>>> Could you explain in a bit detail? Why can't the expected skb be refered
>>> to in next loop?
>>
>>
>> unlink_urbs() complete handler
>> --------------------------------------
>> -------------------------------------------------
>> spin_unlock_irqrestore()
>> rx_complete()
>> derver_bh()
>>
>> __skb_unlink()
>>
>> __skb_queue_tail(&dev->done, skb) =======> skb is moved to
>> dev->done, and can be freed by usbnet_bh()
>> skb_queue_walk_safe()
>> tmp = skb->next ===> refer to freed skb
>
> I see the problem, so looks skb_queue_walk_safe is not safe.
> I don' know why the 2nd ' tmp = skb->next' in skb_queue_walk_safe
> is needed and it may become unsafe if skb is freed during current loop.
>
> But removing the 2nd 'tmp = skb->next' doesn't help the problem, because
> tmp still may become freed after releasing lock.
>
>> If its state is x_done/tx_done/rx_cleanup, that means the the skb will
>> be released soon, right? If so, it should avoid calling
>> usb_unlink_urb().
>
> Even though you can avoid calling unlink for completed URBs, the skbs
> still may be freed in unlink path because complete handler will be triggered
> by unlink and the referenced skb may be freed before next loop, so your
> patch can't fix the oops.
>
Hi Ming,
That's why my patch uses skb_queue_walk() to traverse the queue,
it guarantees the skb available in each loop. Is this what you
expected?
The main idea of my patch(it is based on current mainline: 3.4.0-rc3) is:
1. If the skb in txq/rxq, then it must be available,
unlink_urbs() can refer to it safely while it holds q->lock;
2. If the skb in txq/rxq and its state is
rx_done/tx_done/rx_cleanup, that means the skb's URB is complete, then
don't need to unlink it again;
3. Before releasing q->lock in unlink_urbs(), it will increase
the URB's refercount, so even the related skb is freed in future, the
URB is still available.
Thanks,
--Huajun
> As far as I can think of, we can hold lock of done queue to forbid skb free
> during unlinking. The below patch may fix the problem, are you OK
> with it?
Just skip trying this per your following email's comments.
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index db99536..a9809d4 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, struct
> sk_buff_head *q)
> struct sk_buff *skb, *skbnext;
> int count = 0;
>
> - spin_lock_irqsave (&q->lock, flags);
> + spin_lock_irqsave(&dev->done.lock, flags);
> + spin_lock(&q->lock);
> skb_queue_walk_safe(q, skb, skbnext) {
> struct skb_data *entry;
> struct urb *urb;
> @@ -598,7 +599,7 @@ static int unlink_urbs (struct usbnet *dev, struct
> sk_buff_head *q)
> * handler(include defer_bh).
> */
> usb_get_urb(urb);
> - spin_unlock_irqrestore(&q->lock, flags);
> + spin_unlock(&q->lock);
> // during some PM-driven resume scenarios,
> // these (async) unlinks complete immediately
> retval = usb_unlink_urb (urb);
> @@ -607,9 +608,10 @@ static int unlink_urbs (struct usbnet *dev,
> struct sk_buff_head *q)
> else
> count++;
> usb_put_urb(urb);
> - spin_lock_irqsave(&q->lock, flags);
> + spin_lock(&q->lock);
> }
> - spin_unlock_irqrestore (&q->lock, flags);
> + spin_unlock(&q->lock);
> + spin_unlock_irqrestore(&dev->done.lock, flags);
> return count;
> }
>
>
>
> Thanks,
> --
> Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [net-next 04/14] net: Fix issue with netdev_tx_reset_queue not resetting queue from XOFF state
From: Tom Herbert @ 2012-04-21 6:01 UTC (permalink / raw)
To: Alexander Duyck
Cc: Alexander Duyck, Jeff Kirsher, davem, netdev, gospo, sassmann,
John Fastabend
In-Reply-To: <CAKgT0UcZZNOH=D5mJDaw3_6femW120ZG0cBc8Nz=-=8h6X17kg@mail.gmail.com>
> I don't recall the exact reason now, as John said I think it had to do
> with the ethtool tests not using the same cleanup routine and leaving
> us in a bad state. I am pretty sure there was some path in which
> where the call was didn't work but I do not recall the exact details
> now. Most of the reason for moving it is due to the fact that the
> reset is now also clearing the bit, and from the driver perspective we
> didn't need it in two places. After looking it all over again, I
> suppose this causes a cosmetic issue for the bql "inflight" statistic
> in sysfs since the value will be retained until the interface is
> brought back up with this change. Is that an issue or something that
> can be lived with since the interface isn't active anyway in this
> case?
>
On the surface, it seems cleaner and probably a simpler convention to
clear the state when the buffers are being freed. If there's a good
reason not to do this for igb, it makes me wonder if this should be
done the same way in other drivers...
Tom
^ permalink raw reply
* Re: [PATCH v3.4-rc 0/9] pch_gbe: ptp bug fixes
From: David Miller @ 2012-04-21 5:13 UTC (permalink / raw)
To: richardcochran; +Cc: netdev, tshimizu818
In-Reply-To: <20120421045949.GA16416@netboy.at.omicron.at>
From: Richard Cochran <richardcochran@gmail.com>
Date: Sat, 21 Apr 2012 06:59:49 +0200
> On Fri, Apr 20, 2012 at 08:50:41PM -0400, David Miller wrote:
>>
>> 3.4-rcX is not the appropriate tree to base this work on, whereas
>> net-next is.
>
> Okay, posted again, branched off of net-next.
> Content is the same, I think.
Cool, thanks.
^ permalink raw reply
* Re: [PATCH v3.4-rc 0/9] pch_gbe: ptp bug fixes
From: Richard Cochran @ 2012-04-21 4:59 UTC (permalink / raw)
To: David Miller; +Cc: netdev, tshimizu818
In-Reply-To: <20120420.205041.1617807319412374042.davem@davemloft.net>
On Fri, Apr 20, 2012 at 08:50:41PM -0400, David Miller wrote:
>
> 3.4-rcX is not the appropriate tree to base this work on, whereas
> net-next is.
Okay, posted again, branched off of net-next.
Content is the same, I think.
Thanks,
Richard
^ permalink raw reply
* [PATCH net-next 9/9] pch_gbe: remove suspicious comment
From: Richard Cochran @ 2012-04-21 4:50 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Takahiro Shimizu
In-Reply-To: <cover.1334983471.git.richardcochran@gmail.com>
The time stamping code in this driver appears to have been copied from
the ixp4xx_eth.c driver, including this timing comment. I had actually
measured the time stamp delay on an IXP425, but I really doubt that this
value also applies here.
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index e9b785e..89c6bcf 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -210,7 +210,6 @@ pch_tx_timestamp(struct pch_gbe_adapter *adapter, struct sk_buff *skb)
/*
* This really stinks, but we have to poll for the Tx time stamp.
- * Usually, the time stamp is ready after 4 to 6 microseconds.
*/
for (cnt = 0; cnt < 100; cnt++) {
val = pch_ch_event_read(pdev);
--
1.7.2.5
^ permalink raw reply related
* [PATCH net-next 8/9] pch_gbe: run the ptp bpf just once per packet
From: Richard Cochran @ 2012-04-21 4:50 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Takahiro Shimizu
In-Reply-To: <cover.1334983471.git.richardcochran@gmail.com>
This patch fixes code which needlessly ran the BPF twice per
packet. Instead, we just run the classifier once and test
whether the packet is any kind of PTP event message.
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 53ac2fb..e9b785e 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -134,10 +134,8 @@ static int pch_ptp_match(struct sk_buff *skb, u16 uid_hi, u32 uid_lo, u16 seqid)
u16 *hi, *id;
u32 lo;
- if ((sk_run_filter(skb, ptp_filter) != PTP_CLASS_V2_IPV4) &&
- (sk_run_filter(skb, ptp_filter) != PTP_CLASS_V1_IPV4)) {
+ if (sk_run_filter(skb, ptp_filter) == PTP_CLASS_NONE)
return 0;
- }
offset = ETH_HLEN + IPV4_HLEN(data) + UDP_HLEN;
--
1.7.2.5
^ permalink raw reply related
* [PATCH net-next 7/9] pch_gbe: correct receive time stamp filtering
From: Richard Cochran @ 2012-04-21 4:50 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Takahiro Shimizu
In-Reply-To: <cover.1334983471.git.richardcochran@gmail.com>
From: Takahiro Shimizu <tshimizu818@gmail.com>
This patch fixes the driver so that multicast PTP event messages can
be recognized by the hardware time stamping unit. The station address
register must be set according to the desired transport type.
[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]
Signed-off-by: Takahiro Shimizu <tshimizu818@gmail.com>
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 14 +++++++++++++-
drivers/ptp/Kconfig | 10 +++++++---
2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 799a85a..53ac2fb 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -111,6 +111,9 @@ const char pch_driver_version[] = DRV_VERSION;
/* 0x44 Time Synchronization Channel Event Register Bits */
#define TX_SNAPSHOT_LOCKED (1<<0)
#define RX_SNAPSHOT_LOCKED (1<<1)
+
+#define PTP_L4_MULTICAST_SA "01:00:5e:00:01:81"
+#define PTP_L2_MULTICAST_SA "01:1b:19:00:00:00"
#endif
static unsigned int copybreak __read_mostly = PCH_GBE_COPYBREAK_DEFAULT;
@@ -236,6 +239,7 @@ static int hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
struct hwtstamp_config cfg;
struct pch_gbe_adapter *adapter = netdev_priv(netdev);
struct pci_dev *pdev;
+ u8 station[20];
if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
return -EFAULT;
@@ -269,9 +273,17 @@ static int hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
adapter->hwts_rx_en = 1;
pch_ch_control_write(pdev, MASTER_MODE | CAP_MODE0);
break;
- case HWTSTAMP_FILTER_PTP_V2_EVENT:
+ case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+ adapter->hwts_rx_en = 1;
+ pch_ch_control_write(pdev, V2_MODE | CAP_MODE2);
+ strcpy(station, PTP_L4_MULTICAST_SA);
+ pch_set_station_address(station, pdev);
+ break;
+ case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
adapter->hwts_rx_en = 1;
pch_ch_control_write(pdev, V2_MODE | CAP_MODE2);
+ strcpy(station, PTP_L2_MULTICAST_SA);
+ pch_set_station_address(station, pdev);
break;
default:
return -ERANGE;
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index cd9bc3b..5648dad 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -78,9 +78,13 @@ config PTP_1588_CLOCK_PCH
depends on PCH_GBE
help
This driver adds support for using the PCH EG20T as a PTP
- clock. This clock is only useful if your PTP programs are
- getting hardware time stamps on the PTP Ethernet packets
- using the SO_TIMESTAMPING API.
+ clock. The hardware supports time stamping of PTP packets
+ when using the end-to-end delay (E2E) mechansim. The peer
+ delay mechansim (P2P) is not supported.
+
+ This clock is only useful if your PTP programs are getting
+ hardware time stamps on the PTP Ethernet packets using the
+ SO_TIMESTAMPING API.
To compile this driver as a module, choose M here: the module
will be called ptp_pch.
--
1.7.2.5
^ permalink raw reply related
* [PATCH net-next 6/9] pch_gbe: do not set the channel control register
From: Richard Cochran @ 2012-04-21 4:50 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Takahiro Shimizu
In-Reply-To: <cover.1334983471.git.richardcochran@gmail.com>
From: Takahiro Shimizu <tshimizu818@gmail.com>
We will let the pch_gbe code do that according to the receive time stamp
filter.
[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]
Signed-off-by: Takahiro Shimizu <tshimizu818@gmail.com>
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
drivers/ptp/ptp_pch.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/ptp/ptp_pch.c b/drivers/ptp/ptp_pch.c
index 9e397fe..08c3311 100644
--- a/drivers/ptp/ptp_pch.c
+++ b/drivers/ptp/ptp_pch.c
@@ -652,8 +652,6 @@ pch_probe(struct pci_dev *pdev, const struct pci_device_id *id)
iowrite32(1, &chip->regs->trgt_lo);
iowrite32(0, &chip->regs->trgt_hi);
iowrite32(PCH_TSE_TTIPEND, &chip->regs->event);
- /* Version: IEEE1588 v1 and IEEE1588-2008, Mode: All Evwnt, Locked */
- iowrite32(0x80020000, &chip->regs->ch_control);
pch_eth_enable_set(chip);
--
1.7.2.5
^ permalink raw reply related
* [PATCH net-next 5/9] pch_gbe: improve coding style
From: Richard Cochran @ 2012-04-21 4:50 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Takahiro Shimizu
In-Reply-To: <cover.1334983471.git.richardcochran@gmail.com>
From: Takahiro Shimizu <tshimizu818@gmail.com>
This patch clears up a few coding style issues:
- Makes two function definitions a bit nicer looking.
- Remove unneeded parentheses.
- Simplify macros for register bits.
[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]
Signed-off-by: Takahiro Shimizu <tshimizu818@gmail.com>
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 18 +++++++++---------
1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index dc15e93..799a85a 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -103,9 +103,9 @@ const char pch_driver_version[] = DRV_VERSION;
/* Macros for ieee1588 */
/* 0x40 Time Synchronization Channel Control Register Bits */
#define MASTER_MODE (1<<0)
-#define SLAVE_MODE (0<<0)
+#define SLAVE_MODE (0)
#define V2_MODE (1<<31)
-#define CAP_MODE0 (0<<16)
+#define CAP_MODE0 (0)
#define CAP_MODE2 (1<<17)
/* 0x44 Time Synchronization Channel Event Register Bits */
@@ -151,8 +151,8 @@ static int pch_ptp_match(struct sk_buff *skb, u16 uid_hi, u32 uid_lo, u16 seqid)
seqid == *id);
}
-static void pch_rx_timestamp(
- struct pch_gbe_adapter *adapter, struct sk_buff *skb)
+static void
+pch_rx_timestamp(struct pch_gbe_adapter *adapter, struct sk_buff *skb)
{
struct skb_shared_hwtstamps *shhwtstamps;
struct pci_dev *pdev;
@@ -189,8 +189,8 @@ out:
pch_ch_event_write(pdev, RX_SNAPSHOT_LOCKED);
}
-static void pch_tx_timestamp(
- struct pch_gbe_adapter *adapter, struct sk_buff *skb)
+static void
+pch_tx_timestamp(struct pch_gbe_adapter *adapter, struct sk_buff *skb)
{
struct skb_shared_hwtstamps shhwtstamps;
struct pci_dev *pdev;
@@ -263,15 +263,15 @@ static int hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
break;
case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
adapter->hwts_rx_en = 0;
- pch_ch_control_write(pdev, (SLAVE_MODE | CAP_MODE0));
+ pch_ch_control_write(pdev, SLAVE_MODE | CAP_MODE0);
break;
case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
adapter->hwts_rx_en = 1;
- pch_ch_control_write(pdev, (MASTER_MODE | CAP_MODE0));
+ pch_ch_control_write(pdev, MASTER_MODE | CAP_MODE0);
break;
case HWTSTAMP_FILTER_PTP_V2_EVENT:
adapter->hwts_rx_en = 1;
- pch_ch_control_write(pdev, (V2_MODE | CAP_MODE2));
+ pch_ch_control_write(pdev, V2_MODE | CAP_MODE2);
break;
default:
return -ERANGE;
--
1.7.2.5
^ permalink raw reply related
* [PATCH net-next 4/9] pch_gbe: export a method to set the receive match address
From: Richard Cochran @ 2012-04-21 4:50 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Takahiro Shimizu
In-Reply-To: <cover.1334983471.git.richardcochran@gmail.com>
From: Takahiro Shimizu <tshimizu818@gmail.com>
The code in phc_gbe_main will need to call this method in order to set the
station address register according to the receive time stamping filter.
[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]
Signed-off-by: Takahiro Shimizu <tshimizu818@gmail.com>
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h | 1 +
drivers/ptp/ptp_pch.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
index dd14915..9f3dbc4 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
@@ -660,6 +660,7 @@ extern u32 pch_src_uuid_lo_read(struct pci_dev *pdev);
extern u32 pch_src_uuid_hi_read(struct pci_dev *pdev);
extern u64 pch_rx_snap_read(struct pci_dev *pdev);
extern u64 pch_tx_snap_read(struct pci_dev *pdev);
+extern int pch_set_station_address(u8 *addr, struct pci_dev *pdev);
#endif
/* pch_gbe_param.c */
diff --git a/drivers/ptp/ptp_pch.c b/drivers/ptp/ptp_pch.c
index 847a3c3..9e397fe 100644
--- a/drivers/ptp/ptp_pch.c
+++ b/drivers/ptp/ptp_pch.c
@@ -308,7 +308,7 @@ static void pch_reset(struct pch_dev *chip)
* traffic on the ethernet interface
* @addr: dress which contain the column separated address to be used.
*/
-static int pch_set_station_address(u8 *addr, struct pci_dev *pdev)
+int pch_set_station_address(u8 *addr, struct pci_dev *pdev)
{
s32 i;
struct pch_dev *chip = pci_get_drvdata(pdev);
@@ -352,6 +352,7 @@ static int pch_set_station_address(u8 *addr, struct pci_dev *pdev)
}
return 0;
}
+EXPORT_SYMBOL(pch_set_station_address);
/*
* Interrupt service routine
--
1.7.2.5
^ permalink raw reply related
* [PATCH net-next 3/9] pch_gbe: reprogram multicast address register on reset
From: Richard Cochran @ 2012-04-21 4:50 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Takahiro Shimizu
In-Reply-To: <cover.1334983471.git.richardcochran@gmail.com>
From: Takahiro Shimizu <tshimizu818@gmail.com>
The reset logic after a Rx FIFO overrun will clear the programmed
multicast addresses. This patch fixes the issue by reprogramming the
registers after the reset.
[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]
Signed-off-by: Takahiro Shimizu <tshimizu818@gmail.com>
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 60 +++++++++++++++++++-
1 files changed, 57 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 6a9a63b..dc15e93 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -383,31 +383,85 @@ static void pch_gbe_mac_mar_set(struct pch_gbe_hw *hw, u8 * addr, u32 index)
}
/**
+ * pch_gbe_mac_save_mac_addr_regs - Save MAC addresse registers
+ * @hw: Pointer to the HW structure
+ * @addr: Pointer to the MAC address
+ * @index: MAC address array register
+ */
+static void
+pch_gbe_mac_save_mac_addr_regs(struct pch_gbe_hw *hw,
+ struct pch_gbe_regs_mac_adr *mac_adr, u32 index)
+{
+ mac_adr->high = ioread32(&hw->reg->mac_adr[index].high);
+ mac_adr->low = ioread32(&hw->reg->mac_adr[index].low);
+}
+
+/**
+ * pch_gbe_mac_store_mac_addr_regs - Store MAC addresse registers
+ * @hw: Pointer to the HW structure
+ * @addr: Pointer to the MAC address
+ * @index: MAC address array register
+ */
+static void
+pch_gbe_mac_store_mac_addr_regs(struct pch_gbe_hw *hw,
+ struct pch_gbe_regs_mac_adr *mac_adr, u32 index)
+{
+ u32 adrmask;
+
+ adrmask = ioread32(&hw->reg->ADDR_MASK);
+ iowrite32((adrmask | (0x0001 << index)), &hw->reg->ADDR_MASK);
+ /* wait busy */
+ pch_gbe_wait_clr_bit(&hw->reg->ADDR_MASK, PCH_GBE_BUSY);
+ /* Set the MAC address to the MAC address xA/xB register */
+ iowrite32(mac_adr->high, &hw->reg->mac_adr[index].high);
+ iowrite32(mac_adr->low, &hw->reg->mac_adr[index].low);
+ iowrite32((adrmask & ~(0x0001 << index)), &hw->reg->ADDR_MASK);
+ /* wait busy */
+ pch_gbe_wait_clr_bit(&hw->reg->ADDR_MASK, PCH_GBE_BUSY);
+}
+
+#define MAC_ADDR_LIST_NUM 16
+/**
* pch_gbe_mac_reset_hw - Reset hardware
* @hw: Pointer to the HW structure
*/
static void pch_gbe_mac_reset_hw(struct pch_gbe_hw *hw)
{
+ struct pch_gbe_regs_mac_adr mac_addr_list[MAC_ADDR_LIST_NUM];
+ int i;
+
/* Read the MAC address. and store to the private data */
pch_gbe_mac_read_mac_addr(hw);
+ /* Read other MAC addresses */
+ for (i = 1; i < MAC_ADDR_LIST_NUM; i++)
+ pch_gbe_mac_save_mac_addr_regs(hw, &mac_addr_list[i], i);
iowrite32(PCH_GBE_ALL_RST, &hw->reg->RESET);
#ifdef PCH_GBE_MAC_IFOP_RGMII
iowrite32(PCH_GBE_MODE_GMII_ETHER, &hw->reg->MODE);
#endif
pch_gbe_wait_clr_bit(&hw->reg->RESET, PCH_GBE_ALL_RST);
- /* Setup the receive address */
+ /* Setup the receive addresses */
pch_gbe_mac_mar_set(hw, hw->mac.addr, 0);
+ for (i = 1; i < MAC_ADDR_LIST_NUM; i++)
+ pch_gbe_mac_store_mac_addr_regs(hw, &mac_addr_list[i], i);
return;
}
static void pch_gbe_mac_reset_rx(struct pch_gbe_hw *hw)
{
- /* Read the MAC address. and store to the private data */
+ struct pch_gbe_regs_mac_adr mac_addr_list[MAC_ADDR_LIST_NUM];
+ int i;
+
+ /* Read the MAC addresses. and store to the private data */
pch_gbe_mac_read_mac_addr(hw);
+ for (i = 1; i < MAC_ADDR_LIST_NUM; i++)
+ pch_gbe_mac_save_mac_addr_regs(hw, &mac_addr_list[i], i);
iowrite32(PCH_GBE_RX_RST, &hw->reg->RESET);
pch_gbe_wait_clr_bit_irq(&hw->reg->RESET, PCH_GBE_RX_RST);
- /* Setup the MAC address */
+ /* Setup the MAC addresses */
pch_gbe_mac_mar_set(hw, hw->mac.addr, 0);
+ for (i = 1; i < MAC_ADDR_LIST_NUM; i++)
+ pch_gbe_mac_store_mac_addr_regs(hw, &mac_addr_list[i], i);
return;
}
--
1.7.2.5
^ permalink raw reply related
* [PATCH net-next 2/9] pch_gbe: simplify transmit time stamping flag test
From: Richard Cochran @ 2012-04-21 4:50 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Takahiro Shimizu
In-Reply-To: <cover.1334983471.git.richardcochran@gmail.com>
From: Takahiro Shimizu <tshimizu818@gmail.com>
This patch makes logic surrounding the test of the
transmit time stamping flag more readable.
[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]
Signed-off-by: Takahiro Shimizu <tshimizu818@gmail.com>
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 7c2dabb..6a9a63b 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -199,11 +199,11 @@ static void pch_tx_timestamp(
u32 cnt, val;
shtx = skb_shinfo(skb);
- if (unlikely(shtx->tx_flags & SKBTX_HW_TSTAMP && adapter->hwts_tx_en))
- shtx->tx_flags |= SKBTX_IN_PROGRESS;
- else
+ if (likely(!(shtx->tx_flags & SKBTX_HW_TSTAMP && adapter->hwts_tx_en)))
return;
+ shtx->tx_flags |= SKBTX_IN_PROGRESS;
+
/* Get ieee1588's dev information */
pdev = adapter->ptp_pdev;
--
1.7.2.5
^ permalink raw reply related
* [PATCH net-next 1/9] pch_gbe: scale time stamps to nanoseconds
From: Richard Cochran @ 2012-04-21 4:50 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Takahiro Shimizu
In-Reply-To: <cover.1334983471.git.richardcochran@gmail.com>
From: Takahiro Shimizu <tshimizu818@gmail.com>
This patch fixes the helper functions that give the transmit and
receive time stamps to return nanoseconds, instead of arbitrary clock
ticks.
[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]
Signed-off-by: Takahiro Shimizu <tshimizu818@gmail.com>
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 4 ----
drivers/ptp/ptp_pch.c | 2 ++
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 8035e5f..7c2dabb 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -101,8 +101,6 @@ const char pch_driver_version[] = DRV_VERSION;
#ifdef CONFIG_PCH_PTP
/* Macros for ieee1588 */
-#define TICKS_NS_SHIFT 5
-
/* 0x40 Time Synchronization Channel Control Register Bits */
#define MASTER_MODE (1<<0)
#define SLAVE_MODE (0<<0)
@@ -183,7 +181,6 @@ static void pch_rx_timestamp(
goto out;
ns = pch_rx_snap_read(pdev);
- ns <<= TICKS_NS_SHIFT;
shhwtstamps = skb_hwtstamps(skb);
memset(shhwtstamps, 0, sizeof(*shhwtstamps));
@@ -226,7 +223,6 @@ static void pch_tx_timestamp(
}
ns = pch_tx_snap_read(pdev);
- ns <<= TICKS_NS_SHIFT;
memset(&shhwtstamps, 0, sizeof(shhwtstamps));
shhwtstamps.hwtstamp = ns_to_ktime(ns);
diff --git a/drivers/ptp/ptp_pch.c b/drivers/ptp/ptp_pch.c
index 375eb04..847a3c3 100644
--- a/drivers/ptp/ptp_pch.c
+++ b/drivers/ptp/ptp_pch.c
@@ -261,6 +261,7 @@ u64 pch_rx_snap_read(struct pci_dev *pdev)
ns = ((u64) hi) << 32;
ns |= lo;
+ ns <<= TICKS_NS_SHIFT;
return ns;
}
@@ -277,6 +278,7 @@ u64 pch_tx_snap_read(struct pci_dev *pdev)
ns = ((u64) hi) << 32;
ns |= lo;
+ ns <<= TICKS_NS_SHIFT;
return ns;
}
--
1.7.2.5
^ permalink raw reply related
* [PATCH net-next 0/9] pch_gbe: ptp bug fixes
From: Richard Cochran @ 2012-04-21 4:50 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Takahiro Shimizu
The content in this patch series originally was posted to netdev by
Takahiro on March 23, 2012, as a loose group of five patches. These
patches addressed problems pointed out in review, both on and off
list.
I have taken his work and rebased, squashed, and split it into the
present form, and I also wrote commit messages that more fully explain
the changes. Also, I added the last two patches myself.
Richard Cochran (2):
pch_gbe: run the ptp bpf just once per packet
pch_gbe: remove suspicious comment
Takahiro Shimizu (7):
pch_gbe: scale time stamps to nanoseconds
pch_gbe: simplify transmit time stamping flag test
pch_gbe: reprogram multicast address register on reset
pch_gbe: export a method to set the receive match address
pch_gbe: improve coding style
pch_gbe: do not set the channel control register
pch_gbe: correct receive time stamp filtering
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h | 1 +
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 107 +++++++++++++++-----
drivers/ptp/Kconfig | 10 ++-
drivers/ptp/ptp_pch.c | 7 +-
4 files changed, 95 insertions(+), 30 deletions(-)
--
1.7.2.5
^ permalink raw reply
* Re: [net-next 04/14] net: Fix issue with netdev_tx_reset_queue not resetting queue from XOFF state
From: Alexander Duyck @ 2012-04-21 3:24 UTC (permalink / raw)
To: Tom Herbert
Cc: Alexander Duyck, Jeff Kirsher, davem, netdev, gospo, sassmann,
John Fastabend
In-Reply-To: <4F91D5A2.2060300@intel.com>
On Fri, Apr 20, 2012 at 2:31 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> On 4/20/2012 2:19 PM, Tom Herbert wrote:
>> Hi Jeff,
>>
>>> @@ -3244,7 +3246,6 @@ static void igb_clean_tx_ring(struct igb_ring *tx_ring)
>>> buffer_info = &tx_ring->tx_buffer_info[i];
>>> igb_unmap_and_free_tx_resource(tx_ring, buffer_info);
>>> }
>>> - netdev_tx_reset_queue(txring_txq(tx_ring));
>>>
>> Why is it necessary to remove this? If rings are being freed with
>> going through completion path then we would need this reset. Is this
>> being done elsewhere maybe?
>>
>
> Alex moved this into the igb_configure_tx_ring() iirc to catch an ethtool
> test case. He assured me when I reviewed the patch that igb_configure_tx_ring()
> would always be called before netif_carrier_on() so I think(?) that should
> be OK.
>
> The above removed case was called after netif_carrier_off() anyways.
I don't recall the exact reason now, as John said I think it had to do
with the ethtool tests not using the same cleanup routine and leaving
us in a bad state. I am pretty sure there was some path in which
where the call was didn't work but I do not recall the exact details
now. Most of the reason for moving it is due to the fact that the
reset is now also clearing the bit, and from the driver perspective we
didn't need it in two places. After looking it all over again, I
suppose this causes a cosmetic issue for the bql "inflight" statistic
in sysfs since the value will be retained until the interface is
brought back up with this change. Is that an issue or something that
can be lived with since the interface isn't active anyway in this
case?
Thanks,
Alex
^ permalink raw reply
* Re: use-after-free in usbnet
From: Ming Lei @ 2012-04-21 2:02 UTC (permalink / raw)
To: Huajun Li
Cc: Oliver Neukum, Alan Stern, Dave Jones, netdev, linux-usb,
Fedora Kernel Team
In-Reply-To: <CACVXFVOc0XZ+eLHGiVwKuiUResRk8Cj9MS4EPMx7k57a0tEJhA@mail.gmail.com>
On Sat, Apr 21, 2012 at 9:49 AM, Ming Lei <tom.leiming@gmail.com> wrote:
> On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>> On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>>> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>>>>
>>>> Above patch has already been integrated to mainline. However, maybe
>>>> there still exists another potentail use-after-free issue, here is a
>>>> case:
>>>> After release the lock in unlink_urbs(), defer_bh() may move
>>>> current skb from rxq/txq to dev->done queue, even cause the skb be
>>>> released. Then in next loop cycle, it can't refer to expected skb, and
>>>> may Oops again.
>>>
>>> Could you explain in a bit detail? Why can't the expected skb be refered
>>> to in next loop?
>>
>>
>> unlink_urbs() complete handler
>> --------------------------------------
>> -------------------------------------------------
>> spin_unlock_irqrestore()
>> rx_complete()
>> derver_bh()
>>
>> __skb_unlink()
>>
>> __skb_queue_tail(&dev->done, skb) =======> skb is moved to
>> dev->done, and can be freed by usbnet_bh()
>> skb_queue_walk_safe()
>> tmp = skb->next ===> refer to freed skb
>
> I see the problem, so looks skb_queue_walk_safe is not safe.
> I don' know why the 2nd ' tmp = skb->next' in skb_queue_walk_safe
> is needed and it may become unsafe if skb is freed during current loop.
>
> But removing the 2nd 'tmp = skb->next' doesn't help the problem, because
> tmp still may become freed after releasing lock.
>
>> If its state is x_done/tx_done/rx_cleanup, that means the the skb will
>> be released soon, right? If so, it should avoid calling
>> usb_unlink_urb().
>
> Even though you can avoid calling unlink for completed URBs, the skbs
> still may be freed in unlink path because complete handler will be triggered
> by unlink and the referenced skb may be freed before next loop, so your
> patch can't fix the oops.
>
> As far as I can think of, we can hold lock of done queue to forbid skb free
> during unlinking. The below patch may fix the problem, are you OK
> with it?
Sorry, the patch still doesn't work since done.lock can't be held
before calling unlinking.
One simple solution is just always holding q->lock during the whole loop,
like before commit 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d
(net/usbnet: avoid recursive locking in usbnet_stop()), and take
the per cpu flag trick to avoid holding q->lock in complete handler
called by unlink, see idea in below link:
http://marc.info/?l=linux-usb&m=133491718707499&w=2
In theory, it should be simple, just take avoiding policy.
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index db99536..a9809d4 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, struct
> sk_buff_head *q)
> struct sk_buff *skb, *skbnext;
> int count = 0;
>
> - spin_lock_irqsave (&q->lock, flags);
> + spin_lock_irqsave(&dev->done.lock, flags);
> + spin_lock(&q->lock);
> skb_queue_walk_safe(q, skb, skbnext) {
> struct skb_data *entry;
> struct urb *urb;
> @@ -598,7 +599,7 @@ static int unlink_urbs (struct usbnet *dev, struct
> sk_buff_head *q)
> * handler(include defer_bh).
> */
> usb_get_urb(urb);
> - spin_unlock_irqrestore(&q->lock, flags);
> + spin_unlock(&q->lock);
> // during some PM-driven resume scenarios,
> // these (async) unlinks complete immediately
> retval = usb_unlink_urb (urb);
> @@ -607,9 +608,10 @@ static int unlink_urbs (struct usbnet *dev,
> struct sk_buff_head *q)
> else
> count++;
> usb_put_urb(urb);
> - spin_lock_irqsave(&q->lock, flags);
> + spin_lock(&q->lock);
> }
> - spin_unlock_irqrestore (&q->lock, flags);
> + spin_unlock(&q->lock);
> + spin_unlock_irqrestore(&dev->done.lock, flags);
> return count;
> }
>
>
>
> Thanks,
> --
> Ming Lei
--
Ming Lei
^ permalink raw reply
* Re: use-after-free in usbnet
From: Ming Lei @ 2012-04-21 1:49 UTC (permalink / raw)
To: Huajun Li
Cc: Oliver Neukum, Alan Stern, Dave Jones, netdev, linux-usb,
Fedora Kernel Team
In-Reply-To: <CA+v9cxZWtGbz6uCSysVbAc1hT27rCiuJXzcvSiTxH-zuQYnrZw@mail.gmail.com>
On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
> On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>>>
>>> Above patch has already been integrated to mainline. However, maybe
>>> there still exists another potentail use-after-free issue, here is a
>>> case:
>>> After release the lock in unlink_urbs(), defer_bh() may move
>>> current skb from rxq/txq to dev->done queue, even cause the skb be
>>> released. Then in next loop cycle, it can't refer to expected skb, and
>>> may Oops again.
>>
>> Could you explain in a bit detail? Why can't the expected skb be refered
>> to in next loop?
>
>
> unlink_urbs() complete handler
> --------------------------------------
> -------------------------------------------------
> spin_unlock_irqrestore()
> rx_complete()
> derver_bh()
>
> __skb_unlink()
>
> __skb_queue_tail(&dev->done, skb) =======> skb is moved to
> dev->done, and can be freed by usbnet_bh()
> skb_queue_walk_safe()
> tmp = skb->next ===> refer to freed skb
I see the problem, so looks skb_queue_walk_safe is not safe.
I don' know why the 2nd ' tmp = skb->next' in skb_queue_walk_safe
is needed and it may become unsafe if skb is freed during current loop.
But removing the 2nd 'tmp = skb->next' doesn't help the problem, because
tmp still may become freed after releasing lock.
> If its state is x_done/tx_done/rx_cleanup, that means the the skb will
> be released soon, right? If so, it should avoid calling
> usb_unlink_urb().
Even though you can avoid calling unlink for completed URBs, the skbs
still may be freed in unlink path because complete handler will be triggered
by unlink and the referenced skb may be freed before next loop, so your
patch can't fix the oops.
As far as I can think of, we can hold lock of done queue to forbid skb free
during unlinking. The below patch may fix the problem, are you OK
with it?
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index db99536..a9809d4 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, struct
sk_buff_head *q)
struct sk_buff *skb, *skbnext;
int count = 0;
- spin_lock_irqsave (&q->lock, flags);
+ spin_lock_irqsave(&dev->done.lock, flags);
+ spin_lock(&q->lock);
skb_queue_walk_safe(q, skb, skbnext) {
struct skb_data *entry;
struct urb *urb;
@@ -598,7 +599,7 @@ static int unlink_urbs (struct usbnet *dev, struct
sk_buff_head *q)
* handler(include defer_bh).
*/
usb_get_urb(urb);
- spin_unlock_irqrestore(&q->lock, flags);
+ spin_unlock(&q->lock);
// during some PM-driven resume scenarios,
// these (async) unlinks complete immediately
retval = usb_unlink_urb (urb);
@@ -607,9 +608,10 @@ static int unlink_urbs (struct usbnet *dev,
struct sk_buff_head *q)
else
count++;
usb_put_urb(urb);
- spin_lock_irqsave(&q->lock, flags);
+ spin_lock(&q->lock);
}
- spin_unlock_irqrestore (&q->lock, flags);
+ spin_unlock(&q->lock);
+ spin_unlock_irqrestore(&dev->done.lock, flags);
return count;
}
Thanks,
--
Ming Lei
^ permalink raw reply related
* Re: [PATCH v2] macvlan/macvtap: Fix vlan tagging on user read
From: Eric W. Biederman @ 2012-04-21 1:51 UTC (permalink / raw)
To: Basil Gor; +Cc: netdev, David S. Miller, Basil Gor
In-Reply-To: <1334964058-3338-1-git-send-email-basilgor@gmail.com>
Basil Gor <basil.gor@gmail.com> writes:
> Vlan tag is restored during buffer transmit to a network device (bridge
> port) in bridging code in case of tun/tap driver. In case of macvtap it
> has to be done explicitly. Otherwise vlan_tci is ignored and user always
> gets untagged packets.
>
> Scenario tested:
> kvm guests (that use vlans) migration from bridged network to macvtap
> revealed that packets delivered to guests are always untagged. Dumping
> and comparing sk_buff in case of tap and macvtap driver showed that
> macvtap does not restore vlan_tci.
>
> With current patch applied I was able to get working network, kvm guests
> get correctly tagged packets and can reach each other when macvtap in
> bridge mode (both with no vlans and through vlan interfaces).
>
> Changes from original version:
> vlan header restoring code is moved from macvtap_forward to
> macvtap_receive
I really think this is the wrong fix.
Eric
^ permalink raw reply
* Re: [PATCH] macvlan/macvtap: Fix vlan tagging on user read
From: Eric W. Biederman @ 2012-04-21 1:49 UTC (permalink / raw)
To: Basil Gor; +Cc: netdev, David S. Miller
In-Reply-To: <20120420231128.GA2088@nanobar>
Basil Gor <basil.gor@gmail.com> writes:
> I did some additional code review, and it's easier to show on stack traces and
> by comparing macvtap with tun/tap driver.
>
> tun/tap device does not need to care about vlan tag stuff, as it gets skb with
> vlan id in the header and vlan_tci is not used.
>
> [97493.070321] tun_net_xmit devname vnet0 vlan_tci 0 vlan 0 proto 8100 len 64
> [97493.070327] Pid: 0, comm: swapper/2 Tainted: G O 3.3.1-3.fc16.x86_64 #1
> [97493.070331] Call Trace:
> [97493.070334] <IRQ> [<ffffffffa02c6827>] tun_net_xmit+0x47/0x260 [tun]
> [97493.070347] [<ffffffff814e8072>] dev_hard_start_xmit+0x332/0x6d0 <------ __vlan_put_tag is called
> [97493.070355] [<ffffffff81503f5a>] sch_direct_xmit+0xfa/0x1d0
> [97493.070364] [<ffffffff814e85b5>] dev_queue_xmit+0x1a5/0x640
> [97493.070377] [<ffffffffa057c180>] ? br_flood+0xc0/0xc0 [bridge]
> [97493.070395] [<ffffffffa057c380>] ? __br_deliver+0x100/0x100 [bridge]
> [97493.070409] [<ffffffffa057c380>] ? __br_deliver+0x100/0x100 [bridge]
> [97493.070423] [<ffffffffa057c1ec>] br_dev_queue_push_xmit+0x6c/0xa0 [bridge]
> [97493.070438] [<ffffffffa057c380>] ? __br_deliver+0x100/0x100 [bridge]
> [97493.070457] [<ffffffffa057c242>] br_forward_finish+0x22/0x60 [bridge]
> [97493.070471] [<ffffffffa057c380>] ? __br_deliver+0x100/0x100 [bridge]
> [97493.070485] [<ffffffffa057c3dd>] __br_forward+0x5d/0xb0 [bridge]
> [97493.070495] [<ffffffff814dafe4>] ? skb_clone+0x54/0xb0
> [97493.070508] [<ffffffffa057bf1e>] deliver_clone+0x3e/0x60 [bridge]
> [97493.070523] [<ffffffffa057c143>] br_flood+0x83/0xc0 [bridge]
> [97493.070534] [<ffffffffa057c525>] br_flood_forward+0x15/0x20 [bridge]
> [97493.070544] [<ffffffffa057d256>] br_handle_frame_finish+0x246/0x2a0 [bridge]
> [97493.070555] [<ffffffffa057d444>] br_handle_frame+0x194/0x260 [bridge]
> [97493.070567] [<ffffffffa057d2b0>] ? br_handle_frame_finish+0x2a0/0x2a0 [bridge]
> [97493.070581] [<ffffffff814e56de>] __netif_receive_skb+0x1be/0x5c0 <------ vlan_untag is called
> [97493.070594] [<ffffffff81088ba2>] ? default_wake_function+0x12/0x20
> [97493.070604] [<ffffffff814e5ef1>] process_backlog+0xb1/0x170
> [97493.070613] [<ffffffff814e718b>] net_rx_action+0x12b/0x270
> [97493.070623] [<ffffffff8108daed>] ? sched_clock_cpu+0xbd/0x110
> [97493.070633] [<ffffffff8105efb8>] __do_softirq+0xb8/0x230
> [97493.070644] [<ffffffff810e3c30>] ? handle_irq_event+0x50/0x70
> [97493.070654] [<ffffffff815fd49c>] call_softirq+0x1c/0x30
> [97493.070662] [<ffffffff81016455>] do_softirq+0x65/0xa0
> [97493.070668] [<ffffffff8105f3ce>] irq_exit+0x9e/0xc0
> [97493.070675] [<ffffffff815fdd03>] do_IRQ+0x63/0xe0
> [97493.070682] [<ffffffff815f42ae>] common_interrupt+0x6e/0x6e
> [97493.070686] <EOI> [<ffffffff8131b236>] ? intel_idle+0xe6/0x150
> [97493.070697] [<ffffffff8131b218>] ? intel_idle+0xc8/0x150
> [97493.070705] [<ffffffff814a4071>] cpuidle_idle_call+0xc1/0x280
> [97493.070713] [<ffffffff8101322f>] cpu_idle+0xcf/0x120
> [97493.070720] [<ffffffff815e3b1f>] start_secondary+0x282/0x284
>
> but macvtap device gets skb with vlan tag extacted in vlan_tci, and as original driver
> code was mostly based on tun/tap driver vlan thing was missed.
>
> [98143.863560] macvtap_receive devname macvtap0 vlan_tci 100a vlan 10 proto 0800 len 90
> [98143.863570] macvtap_forward devname macvtap0 vlan_tci 100a vlan 10 proto 0800 len 90
> [98143.863578] Pid: 0, comm: swapper/2 Tainted: G O 3.3.1-3.fc16.x86_64 #1
> [98143.863583] Call Trace:
> [98143.863587] <IRQ> [<ffffffffa026819c>] macvtap_forward+0x8c/0x1b0 [macvtap]
> [98143.863606] [<ffffffffa0268314>] macvtap_receive+0x54/0x60 [macvtap]
> [98143.863623] [<ffffffffa02c05db>] macvlan_handle_frame+0xbb/0x2c0 [macvlan]
> [98143.863635] [<ffffffffa02c0520>] ? macvlan_broadcast+0x160/0x160 [macvlan]
> [98143.863646] [<ffffffff814e56de>] __netif_receive_skb+0x1be/0x5c0 <------ vlan_untag is called
> [98143.863653] [<ffffffff814e67e3>] netif_receive_skb+0x23/0x90
> [98143.863660] [<ffffffff814e6c09>] ? dev_gro_receive+0x1b9/0x2b0
> [98143.863667] [<ffffffff814e6950>] napi_skb_finish+0x50/0x70
> [98143.863673] [<ffffffff814e6f45>] napi_gro_receive+0xf5/0x140
> [98143.863697] [<ffffffffa0241fab>] e1000_receive_skb+0x5b/0x70 [e1000e]
Actually __vlan_hwaccel_put_tag is called here not vlan_untag
> [98143.863718] [<ffffffffa0244b21>] e1000_clean_rx_irq+0x2f1/0x400 [e1000e]
> [98143.863737] [<ffffffffa02432e8>] e1000_clean+0x78/0x2c0 [e1000e]
> [98143.863745] [<ffffffff814e718b>] net_rx_action+0x12b/0x270
> [98143.863752] [<ffffffff8108daed>] ? sched_clock_cpu+0xbd/0x110
> [98143.863759] [<ffffffff8105efb8>] __do_softirq+0xb8/0x230
> [98143.863767] [<ffffffff810e3c30>] ? handle_irq_event+0x50/0x70
> [98143.863775] [<ffffffff815fd49c>] call_softirq+0x1c/0x30
> [98143.863782] [<ffffffff81016455>] do_softirq+0x65/0xa0
> [98143.863788] [<ffffffff8105f3ce>] irq_exit+0x9e/0xc0
> [98143.863796] [<ffffffff815fdd03>] do_IRQ+0x63/0xe0
> [98143.863803] [<ffffffff815f42ae>] common_interrupt+0x6e/0x6e
> [98143.863807] <EOI> [<ffffffff8131b236>] ? intel_idle+0xe6/0x150
> [98143.863818] [<ffffffff8131b218>] ? intel_idle+0xc8/0x150
> [98143.863826] [<ffffffff814a4071>] cpuidle_idle_call+0xc1/0x280
> [98143.863834] [<ffffffff8101322f>] cpu_idle+0xcf/0x120
> [98143.863841] [<ffffffff815e3b1f>] start_secondary+0x282/0x284
>
> and as Eric Biederman noted, why not add vlan header back at the last moment? in
> macvtap_put_user. And it would work for user space applications which read
> /dev/tapX, but in kvm case actual reading is done by vhost_net driver. And this
> driver actually does skb_peek on macvtap queue to get next packet size before
> reading (in handle_rx).
>
> [98143.863878] vhost peek_head_len devname macvtap0 vlan_tci 100a vlan 10 proto 0800 len 90
>
> so, it gets skb len without vlan tag and then performs read with buffer smaller then needed
>
> [98143.863885] macvtap_do_read buflen 102 <--- 90 + (vnet_hdr_sz 12 bytes)
> [98143.863889] devname macvtap0 vlan_tci 100a vlan 10 proto 0800 len 90
> __vlan_put_tag is called here
> [98143.863894] macvtap_do_read reallen 106 <--- 90 + 4 + (vnet_hdr_sz 12)
> [98143.863898] devname macvtap0 vlan_tci 0 vlan 0 proto 8100 len 94
> [98143.863904] Pid: 7289, comm: vhost-7236 Tainted: G O 3.3.1-3.fc16.x86_64 #1
> [98143.863935] Call Trace:
> [98143.863944] [<ffffffffa0268593>] macvtap_do_read+0x243/0x420 [macvtap]
> [98143.863954] [<ffffffff81088b90>] ? try_to_wake_up+0x2b0/0x2b0
> [98143.863962] [<ffffffffa02687ba>] macvtap_recvmsg+0x4a/0x70 [macvtap]
> [98143.863971] [<ffffffffa02e149e>] handle_rx+0x39e/0x6e0 [vhost_net]
> [98143.863983] [<ffffffffa02e17f5>] handle_rx_net+0x15/0x20 [vhost_net]
> [98143.863996] [<ffffffffa02de84c>] vhost_worker+0xcc/0x150 [vhost_net]
> [98143.864008] [<ffffffffa02de780>] ? __vhost_add_used_n+0x110/0x110 [vhost_net]
> [98143.864020] [<ffffffff81079af3>] kthread+0x93/0xa0
> [98143.864032] [<ffffffff815fd3a4>] kernel_thread_helper+0x4/0x10
> [98143.864044] [<ffffffff81079a60>] ? kthread_freezable_should_stop+0x70/0x70
> [98143.864056] [<ffffffff815fd3a0>] ? gs_change+0x13/0x13
>
> things get more interesting when we take another case in account. When one kvm guest sends
> packet on the same macvlan to another guest macvtap gets skb with vlan id in the header
> and vlan_tci is not used.
>
> [99564.523943] macvtap_forward devname (null) vlan_tci 0 vlan 0 proto 8100 len 94
> [99564.523946] Pid: 8849, comm: vhost-8797 Tainted: G O 3.3.1-3.fc16.x86_64 #1
> [99564.523947] Call Trace:
> [99564.523952] [<ffffffffa02de19c>] macvtap_forward+0x8c/0x1b0 [macvtap]
> [99564.523963] [<ffffffffa02c0502>] macvlan_broadcast+0x142/0x160 [macvlan]
> [99564.523967] [<ffffffffa02c146d>] macvlan_start_xmit+0x14d/0x178 [macvlan]
> [99564.523969] [<ffffffffa02df378>] macvtap_get_user+0x388/0x420 [macvtap]
> [99564.523971] [<ffffffffa02df43b>] macvtap_sendmsg+0x2b/0x30 [macvtap]
> [99564.523973] [<ffffffffa026bb3d>] handle_tx+0x2dd/0x620 [vhost_net]
> [99564.523976] [<ffffffffa026beb5>] handle_tx_kick+0x15/0x20 [vhost_net]
> [99564.523978] [<ffffffffa026884c>] vhost_worker+0xcc/0x150 [vhost_net]
> [99564.523980] [<ffffffffa0268780>] ? __vhost_add_used_n+0x110/0x110 [vhost_net]
> [99564.523984] [<ffffffff81079af3>] kthread+0x93/0xa0
> [99564.523987] [<ffffffff815fd3a4>] kernel_thread_helper+0x4/0x10
> [99564.523989] [<ffffffff81079a60>] ? kthread_freezable_should_stop+0x70/0x70
> [99564.523991] [<ffffffff815fd3a0>] ? gs_change+0x13/0x13
> [99564.523999] vhost peek_head_len devname (null) vlan_tci 0 vlan 0 proto 8100 len 94
> [99564.524003] macvtap_do_read buflen 106
> [99564.524004] macvtap_do_read reallen 106
>
> And we definitely want to have common rules for all cases. So we either
> 1. restore vlan headers from vlan_tci for any packets coming outside of macvlan in
> macvtap_receive and we don't need to fix vhost_net and we preserve same vlan id
> policy that tun/tap driver have. (my original patch)
> or
> 2. we extract vlan ids for packets coming from the same macvlan, fixing vhost_net to
> take vlan_tci into account and restoring vlan headers on
> macvtap_put_user
And 2 seems to be the right answer.
Not long ago we went a few rounds with this with the core parts of the
networking stack and the rule that evolved was that we always strip the
vlan tag. Which is why we have the vlan_untag call in __netif_receive_skb()
to handle the small handful of networking devices that don't.
vhost/net.c is just buggy. PF_PACKET sockets have been returning
the vlan_id in ancillary data from year hardware like the e1000 since
at least 2005.
So I agree that both macvtap and vhost/net.c both need to be fixed.
Eric
^ permalink raw reply
* Re: [PATCH net-next 00/19] net: Sysctl simplifications and enhancements
From: David Miller @ 2012-04-21 1:24 UTC (permalink / raw)
To: xemul; +Cc: ebiederm, netdev, serge, gaofeng, pablo, shemminger
In-Reply-To: <4F912240.8010802@parallels.com>
From: Pavel Emelyanov <xemul@parallels.com>
Date: Fri, 20 Apr 2012 12:45:52 +0400
> On 04/20/2012 03:17 AM, Eric W. Biederman wrote:
>>
>> Summary:
>> - Kill approximately 400 lines of code
>> - Allow all networking sysctls with just CAP_NET_ADMIN
>> - Hide all networking sysctls that don't apply to your current network namespace.
>> - Uniformly register flat sysctl tables not sysctl tables with .child entries
>> - Readable string paths for registering sysctls
>>
>> Eric W. Biederman (19):
>> net: Implement register_net_sysctl.
>> net sysctl: Register an empty /proc/sys/net
>> net sysctl: Initialize the network sysctls sooner to avoid problems.
>> net: Kill register_sysctl_rotable
>> net: Move all of the network sysctls without a namespace into init_net.
>> net core: Remove unneded creation of an empty net/core sysctl directory
>> net ipv6: Remove unneded registration of an empty net/ipv6/neigh
>> net ipv4: Remove the unneeded registration of an empty net/ipv4/neigh
>> net ax25: Simplify and cleanup the ax25 sysctl handling.
>> net llc: Don't use sysctl tables with .child entries.
>> net ipv6: Don't use sysctl tables with .child entries.
>> net neighbour: Convert to use register_net_sysctl
>> net decnet: Convert to use register_net_sysctl
>> net ipv6: Convert addrconf to use register_net_sysctl
>> net ipv4: Convert devinet to use register_net_sysctl
>> net: Convert nf_conntrack_proto to use register_net_sysctl
>> net: Convert all sysctl registrations to register_net_sysctl
>> net: Delete all remaining instances of ctl_path
>> net: Remove register_net_sysctl_table
>
> After resolving issues with Eric
>
> Acked-by: Pavel Emelyanov <xemul@parallels.com>
Series applied, thanks everyone.
^ 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