* Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask
From: Michael S. Tsirkin @ 2013-08-15 19:09 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
In-Reply-To: <520D21CA.4090400@redhat.com>
On Thu, Aug 15, 2013 at 02:45:30PM -0400, Vlad Yasevich wrote:
> On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
> >On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
> >>The features of the macvlan are based on the features of lower
> >>device and thus can have checksum featurs other then IFF_F_HW_CSUM
> >
> >s/featurs/features/
> >
> >:set spell spelllang=en_us
> >
> >or whatever's the equivalent in your editor of choice.
> >
> >>set. However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM. Thus
> >>when performing gso segmentation during macvtap_forward(),
> >>it is possbile to end up with skbs that have ip_summed set
> >>to CHECKSUM_PARTIAL. This is incorrect when the user
> >>turns off checksum offloading.
> >>
> >>Include all possible checksum offload values so that
> >>we'll properly mask them off when performing GSO.
> >>
> >>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>---
> >> drivers/net/macvtap.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >>index b51db2a..8121358 100644
> >>--- a/drivers/net/macvtap.c
> >>+++ b/drivers/net/macvtap.c
> >>@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
> >>
> >> static const struct proto_ops macvtap_socket_ops;
> >>
> >>-#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >>+#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >> NETIF_F_TSO6 | NETIF_F_UFO)
> >> #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
> >> /*
> >
> >Okay so you are talking about hardware that sets some other
> >checksum bit besides HW_CSUM, e.g. IP_CSUM, so
> >
> > vlan->tap_features = vlan->dev->features &
> > (feature_mask | ~TUN_OFFLOADS);
> >
> >will not clear IP_CSUM even if feature_mask is 0.
> >
> >Maybe mention this in the changelog, in case user
> >will wonder whether his hardware is affected.
> >
> >So I agree, that's a bug, but if you make this change the reverse will hold
> >(on this hardware):
> >if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
> >so checksum offloading won't work.
> >
> >So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
> >and not just in this place.
>
> Yes. I thought about that, but forgot to do in this patch set.
In fact I wonder why do we do it like this at all.
tap_features have nothing to do with the device,
they only have to do with tap.
For example, you might have a dumb device with no
checksum support but userspace said it does not need a checksum,
we can have NETIF_F_HW_CSUM there anyway.
So why don't we just
vlan->tap_features = feature_mask;
?
In fact, this is exactly set_features, so why don't
we just use set_features everywhere?
Then this patch won't be needed at all.
Kind of like this - compile-tested only - do you have a setup with
IP_CSUM which you can use to test?
-->
macvtap: fix up tap features
There's no apparent need to have separate tap features
masked according to the lowerdev config:
we only use them in software so hardware configuration
does not matter.
Drop tap_features and use set_features directly
This fixes bugs for some unusual hardware configurations,
for example, we only masked HW_CSUM so for hardware with
e.g. IP_CSUM the checksum bit remained set, which would cause
packets with CHECKSUM_PARTIAL to be sent to userspace
even when offloads are disabled.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index a98ed9f..26cea91 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -289,7 +289,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
/* Apply the forward feature mask so that we perform segmentation
* according to users wishes.
*/
- features = netif_skb_features(skb) & vlan->tap_features;
+ features = netif_skb_features(skb) & vlan->set_features;
if (netif_needs_gso(skb, features)) {
struct sk_buff *segs = __skb_gso_segment(skb, features, false);
@@ -382,11 +382,6 @@ static int macvtap_newlink(struct net *src_net,
struct macvlan_dev *vlan = netdev_priv(dev);
INIT_LIST_HEAD(&vlan->queue_list);
- /* Since macvlan supports all offloads by default, make
- * tap support all offloads also.
- */
- vlan->tap_features = TUN_OFFLOADS;
-
/* Don't put anything that may fail after macvlan_common_newlink
* because we can't undo what it does.
*/
@@ -1027,7 +1022,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
features = vlan->dev->features;
if (arg & TUN_F_CSUM) {
- feature_mask = NETIF_F_HW_CSUM;
+ feature_mask = NETIF_F_HW_CSUM;
if (arg & (TUN_F_TSO4 | TUN_F_TSO6)) {
if (arg & TUN_F_TSO_ECN)
@@ -1055,11 +1050,6 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
else
features &= ~RX_OFFLOADS;
- /* tap_features are the same as features on tun/tap and
- * reflect user expectations.
- */
- vlan->tap_features = vlan->dev->features &
- (feature_mask | ~TUN_OFFLOADS);
vlan->set_features = features;
netdev_update_features(vlan->dev);
diff --git a/include/linux/if_macvlan.h b/include/linux/if_macvlan.h
index ddd33fd..e446e82 100644
--- a/include/linux/if_macvlan.h
+++ b/include/linux/if_macvlan.h
@@ -76,7 +76,6 @@ struct macvlan_dev {
struct list_head queue_list;
int numvtaps;
int numqueues;
- netdev_features_t tap_features;
int minor;
};
^ permalink raw reply related
* Re: [PATCH 1/3 v4] ipv6: do not disable temp_address when reaching max_address
From: Hannes Frederic Sowa @ 2013-08-15 19:07 UTC (permalink / raw)
To: Ding Tianhong, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Jon Maloy, Eric Dumazet,
Netdev, kargig, ppandit
In-Reply-To: <20130815173603.GA30746@order.stressinduktion.org>
On Thu, Aug 15, 2013 at 07:36:03PM +0200, Hannes Frederic Sowa wrote:
> Now we have to check why these addresses don't go out of tentative state.
Just looked at it. flood_router26 just emits pretty high values for
RetransTime:
21:05:52.050159 IP6 (hlim 255, next-header ICMPv6 (58) payload length: 192) fe80::c:2a47:1360:1101 > ff02::1: [icmp6 sum ok] ICMP6, router advertisement, length 192
hop limit 255, Flags [none], pref high, router lifetime 65535s, reachable time 16384000ms, retrans time 1966080ms
mtu option (5), length 8 (1): 1500
0x0000: 0000 0000 05dc
We are completly in spec here. Just DAD needs a lot of time to finish.
Greetings,
Hannes
^ permalink raw reply
* Re: [PATCH v3 2/2] macvtap: Allow tap features change when IFF_VNET_HDR is disabled.
From: Vlad Yasevich @ 2013-08-15 18:59 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev
In-Reply-To: <20130815184844.GB11350@redhat.com>
On 08/15/2013 02:48 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 15, 2013 at 02:39:39PM -0400, Vlad Yasevich wrote:
>> On 08/15/2013 01:33 PM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 15, 2013 at 01:02:53PM -0400, Vlad Yasevich wrote:
>>>> When the user turns off IFF_VNET_HDR flag, attempts to change
>>>> offload features via TUNSETOFFLOAD do not work. This could cause
>>>> GSO packets to be delivered to the user when the user is
>>>> not prepared to handle them.
>>>>
>>>> To solve, allow processing of TUNSETOFFLOAD when IFF_VNET_HDR is
>>>> disabled. Also, take the setting of IFF_VNET_HDR into consideration
>>>> when determining the feature set to apply to incommig traffic.
>>>> If IFF_VNET_HDR is set, we use user-specfied feature set. if the
>>>> IFF_VNET_HDR is clear, we mask off all tun-specific offload features,
>>>> since user can not be notified of the possbile offloads.
>>>>
>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>>> ---
>>>> drivers/net/macvtap.c | 30 ++++++++++++++++++++++++------
>>>> 1 file changed, 24 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>> index 8121358..df45a59 100644
>>>> --- a/drivers/net/macvtap.c
>>>> +++ b/drivers/net/macvtap.c
>>>> @@ -269,6 +269,28 @@ static void macvtap_del_queues(struct net_device *dev)
>>>> sock_put(&qlist[j]->sk);
>>>> }
>>>>
>>>> +/* Determine features to be applied to the packet as it
>>>
>>> /*
>>> * Please use comments
>>> * like this
>>> */
>>>
>>>> + * gets sent to the tap. The features depend on the offloads
>>>> + * set by user and by whether or not IFF_VNET_HDR is enabled.
>>>> + */
>>>> +static netdev_features_t macvtap_skb_features(struct sk_buff *skb,
>>>> + struct macvtap_queue *q)
>>>> +{
>>>> + struct macvlan_dev *vlan = netdev_priv(skb->dev);
>>>> + netdev_features_t features = netif_skb_features(skb);
>>>> +
>>>> + /* If VNET_HDR is enabled, user can receive offload info so
>>>> + * use user-specified tap_features.
>>>> + * Otherwise, mask off all tap offload features.
>>>> + */
>>>> + if (q->flags & IFF_VNET_HDR)
>>>> + features &= vlan->tap_features;
>>>> + else
>>>> + features &= ~TUN_OFFLOADS;
>>>> +
>>>> + return features;
>>>> +}
>>>> +
>>>
>>> Okay, but this means we need to do netdev_update_features
>>> on SETIFF since it changes q->flags.
>>
>> Why?
>> The whole point it not to change features on SETIFF, so that
>> they are preserved for when the flags change again. It just that
>> flag values either allow whatever offload user specified (correctly
>> masked via netdev_update_features) or turn off all offloads at the tap.
>>
>> -vlad
>
> That's the point. As it is, if flag value changes, we don't call
> netdev_update_features so offloads aren't masked/unmasked.
>
> Example:
>
> 1. SETOFFLOADS to enable
> 2. VNET = 0
>
> offloads are still enabled
>
But unused since since VNET=0. I am trying to be careful not to
break things when ioctl calls get all inverted and reversed, but
may be I am trying too hard and we should just let things break.
I just feels kinds of silly since the app may now know all the
details, but the kernel does and can do the "right thing".
-vlad
>
>>>> /*
>>>> * Forward happens for data that gets sent from one macvlan
>>>> * endpoint to another one in bridge mode. We just take
>>>> @@ -276,9 +298,9 @@ static void macvtap_del_queues(struct net_device *dev)
>>>> */
>>>> static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
>>>> {
>>>> - struct macvlan_dev *vlan = netdev_priv(dev);
>>>> struct macvtap_queue *q = macvtap_get_queue(dev, skb);
>>>> netdev_features_t features;
>>>> +
>>>> if (!q)
>>>> goto drop;
>>>>
>>>> @@ -289,7 +311,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
>>>> /* Apply the forward feature mask so that we perform segmentation
>>>> * according to users wishes.
>>>> */
>>>> - features = netif_skb_features(skb) & vlan->tap_features;
>>>> + features = macvtap_skb_features(skb, q);
>>>> if (netif_needs_gso(skb, features)) {
>>>> struct sk_buff *segs = __skb_gso_segment(skb, features, false);
>>>>
>>>> @@ -1161,10 +1183,6 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>>>> TUN_F_TSO_ECN | TUN_F_UFO))
>>>> return -EINVAL;
>>>>
>>>> - /* TODO: only accept frames with the features that
>>>> - got enabled for forwarded frames */
>>>> - if (!(q->flags & IFF_VNET_HDR))
>>>> - return -EINVAL;
>>>> rtnl_lock();
>>>> ret = set_offload(q, arg);
>>>> rtnl_unlock();
>>>> --
>>>> 1.8.1.4
^ permalink raw reply
* Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask
From: Michael S. Tsirkin @ 2013-08-15 18:52 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
In-Reply-To: <520D21CA.4090400@redhat.com>
On Thu, Aug 15, 2013 at 02:45:30PM -0400, Vlad Yasevich wrote:
> On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
> >On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
> >>The features of the macvlan are based on the features of lower
> >>device and thus can have checksum featurs other then IFF_F_HW_CSUM
> >
> >s/featurs/features/
> >
> >:set spell spelllang=en_us
> >
> >or whatever's the equivalent in your editor of choice.
> >
> >>set. However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM. Thus
> >>when performing gso segmentation during macvtap_forward(),
> >>it is possbile to end up with skbs that have ip_summed set
> >>to CHECKSUM_PARTIAL. This is incorrect when the user
> >>turns off checksum offloading.
> >>
> >>Include all possible checksum offload values so that
> >>we'll properly mask them off when performing GSO.
> >>
> >>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>---
> >> drivers/net/macvtap.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >>index b51db2a..8121358 100644
> >>--- a/drivers/net/macvtap.c
> >>+++ b/drivers/net/macvtap.c
> >>@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
> >>
> >> static const struct proto_ops macvtap_socket_ops;
> >>
> >>-#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >>+#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >> NETIF_F_TSO6 | NETIF_F_UFO)
> >> #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
> >> /*
> >
> >Okay so you are talking about hardware that sets some other
> >checksum bit besides HW_CSUM, e.g. IP_CSUM, so
> >
> > vlan->tap_features = vlan->dev->features &
> > (feature_mask | ~TUN_OFFLOADS);
> >
> >will not clear IP_CSUM even if feature_mask is 0.
> >
> >Maybe mention this in the changelog, in case user
> >will wonder whether his hardware is affected.
> >
> >So I agree, that's a bug, but if you make this change the reverse will hold
> >(on this hardware):
> >if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
> >so checksum offloading won't work.
> >
> >So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
> >and not just in this place.
>
> Yes. I thought about that, but forgot to do in this patch set.
>
> >
> >Also - Cc stable?
> >
>
> The offloads work went into 3.11, so I don't think there is a need
> for stable.
Right. Thanks for the correction.
> -vlad
> >
> >>--
> >>1.8.1.4
^ permalink raw reply
* Re: [PATCH v3 2/2] macvtap: Allow tap features change when IFF_VNET_HDR is disabled.
From: Michael S. Tsirkin @ 2013-08-15 18:48 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
In-Reply-To: <520D206B.3000203@redhat.com>
On Thu, Aug 15, 2013 at 02:39:39PM -0400, Vlad Yasevich wrote:
> On 08/15/2013 01:33 PM, Michael S. Tsirkin wrote:
> >On Thu, Aug 15, 2013 at 01:02:53PM -0400, Vlad Yasevich wrote:
> >>When the user turns off IFF_VNET_HDR flag, attempts to change
> >>offload features via TUNSETOFFLOAD do not work. This could cause
> >>GSO packets to be delivered to the user when the user is
> >>not prepared to handle them.
> >>
> >>To solve, allow processing of TUNSETOFFLOAD when IFF_VNET_HDR is
> >>disabled. Also, take the setting of IFF_VNET_HDR into consideration
> >>when determining the feature set to apply to incommig traffic.
> >>If IFF_VNET_HDR is set, we use user-specfied feature set. if the
> >>IFF_VNET_HDR is clear, we mask off all tun-specific offload features,
> >>since user can not be notified of the possbile offloads.
> >>
> >>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>---
> >> drivers/net/macvtap.c | 30 ++++++++++++++++++++++++------
> >> 1 file changed, 24 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >>index 8121358..df45a59 100644
> >>--- a/drivers/net/macvtap.c
> >>+++ b/drivers/net/macvtap.c
> >>@@ -269,6 +269,28 @@ static void macvtap_del_queues(struct net_device *dev)
> >> sock_put(&qlist[j]->sk);
> >> }
> >>
> >>+/* Determine features to be applied to the packet as it
> >
> >/*
> > * Please use comments
> > * like this
> > */
> >
> >>+ * gets sent to the tap. The features depend on the offloads
> >>+ * set by user and by whether or not IFF_VNET_HDR is enabled.
> >>+ */
> >>+static netdev_features_t macvtap_skb_features(struct sk_buff *skb,
> >>+ struct macvtap_queue *q)
> >>+{
> >>+ struct macvlan_dev *vlan = netdev_priv(skb->dev);
> >>+ netdev_features_t features = netif_skb_features(skb);
> >>+
> >>+ /* If VNET_HDR is enabled, user can receive offload info so
> >>+ * use user-specified tap_features.
> >>+ * Otherwise, mask off all tap offload features.
> >>+ */
> >>+ if (q->flags & IFF_VNET_HDR)
> >>+ features &= vlan->tap_features;
> >>+ else
> >>+ features &= ~TUN_OFFLOADS;
> >>+
> >>+ return features;
> >>+}
> >>+
> >
> >Okay, but this means we need to do netdev_update_features
> >on SETIFF since it changes q->flags.
>
> Why?
> The whole point it not to change features on SETIFF, so that
> they are preserved for when the flags change again. It just that
> flag values either allow whatever offload user specified (correctly
> masked via netdev_update_features) or turn off all offloads at the tap.
>
> -vlad
That's the point. As it is, if flag value changes, we don't call
netdev_update_features so offloads aren't masked/unmasked.
Example:
1. SETOFFLOADS to enable
2. VNET = 0
offloads are still enabled
> >> /*
> >> * Forward happens for data that gets sent from one macvlan
> >> * endpoint to another one in bridge mode. We just take
> >>@@ -276,9 +298,9 @@ static void macvtap_del_queues(struct net_device *dev)
> >> */
> >> static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
> >> {
> >>- struct macvlan_dev *vlan = netdev_priv(dev);
> >> struct macvtap_queue *q = macvtap_get_queue(dev, skb);
> >> netdev_features_t features;
> >>+
> >> if (!q)
> >> goto drop;
> >>
> >>@@ -289,7 +311,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
> >> /* Apply the forward feature mask so that we perform segmentation
> >> * according to users wishes.
> >> */
> >>- features = netif_skb_features(skb) & vlan->tap_features;
> >>+ features = macvtap_skb_features(skb, q);
> >> if (netif_needs_gso(skb, features)) {
> >> struct sk_buff *segs = __skb_gso_segment(skb, features, false);
> >>
> >>@@ -1161,10 +1183,6 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
> >> TUN_F_TSO_ECN | TUN_F_UFO))
> >> return -EINVAL;
> >>
> >>- /* TODO: only accept frames with the features that
> >>- got enabled for forwarded frames */
> >>- if (!(q->flags & IFF_VNET_HDR))
> >>- return -EINVAL;
> >> rtnl_lock();
> >> ret = set_offload(q, arg);
> >> rtnl_unlock();
> >>--
> >>1.8.1.4
^ permalink raw reply
* Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask
From: Vlad Yasevich @ 2013-08-15 18:45 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev
In-Reply-To: <20130815182430.GB10265@redhat.com>
On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
>> The features of the macvlan are based on the features of lower
>> device and thus can have checksum featurs other then IFF_F_HW_CSUM
>
> s/featurs/features/
>
> :set spell spelllang=en_us
>
> or whatever's the equivalent in your editor of choice.
>
>> set. However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM. Thus
>> when performing gso segmentation during macvtap_forward(),
>> it is possbile to end up with skbs that have ip_summed set
>> to CHECKSUM_PARTIAL. This is incorrect when the user
>> turns off checksum offloading.
>>
>> Include all possible checksum offload values so that
>> we'll properly mask them off when performing GSO.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>> drivers/net/macvtap.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>> index b51db2a..8121358 100644
>> --- a/drivers/net/macvtap.c
>> +++ b/drivers/net/macvtap.c
>> @@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
>>
>> static const struct proto_ops macvtap_socket_ops;
>>
>> -#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
>> +#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
>> NETIF_F_TSO6 | NETIF_F_UFO)
>> #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
>> /*
>
> Okay so you are talking about hardware that sets some other
> checksum bit besides HW_CSUM, e.g. IP_CSUM, so
>
> vlan->tap_features = vlan->dev->features &
> (feature_mask | ~TUN_OFFLOADS);
>
> will not clear IP_CSUM even if feature_mask is 0.
>
> Maybe mention this in the changelog, in case user
> will wonder whether his hardware is affected.
>
> So I agree, that's a bug, but if you make this change the reverse will hold
> (on this hardware):
> if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
> so checksum offloading won't work.
>
> So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
> and not just in this place.
Yes. I thought about that, but forgot to do in this patch set.
>
> Also - Cc stable?
>
The offloads work went into 3.11, so I don't think there is a need for
stable.
-vlad
>
>> --
>> 1.8.1.4
^ permalink raw reply
* Re: [PATCH v3 2/2] macvtap: Allow tap features change when IFF_VNET_HDR is disabled.
From: Vlad Yasevich @ 2013-08-15 18:39 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev
In-Reply-To: <20130815173347.GA10265@redhat.com>
On 08/15/2013 01:33 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 15, 2013 at 01:02:53PM -0400, Vlad Yasevich wrote:
>> When the user turns off IFF_VNET_HDR flag, attempts to change
>> offload features via TUNSETOFFLOAD do not work. This could cause
>> GSO packets to be delivered to the user when the user is
>> not prepared to handle them.
>>
>> To solve, allow processing of TUNSETOFFLOAD when IFF_VNET_HDR is
>> disabled. Also, take the setting of IFF_VNET_HDR into consideration
>> when determining the feature set to apply to incommig traffic.
>> If IFF_VNET_HDR is set, we use user-specfied feature set. if the
>> IFF_VNET_HDR is clear, we mask off all tun-specific offload features,
>> since user can not be notified of the possbile offloads.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>> drivers/net/macvtap.c | 30 ++++++++++++++++++++++++------
>> 1 file changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>> index 8121358..df45a59 100644
>> --- a/drivers/net/macvtap.c
>> +++ b/drivers/net/macvtap.c
>> @@ -269,6 +269,28 @@ static void macvtap_del_queues(struct net_device *dev)
>> sock_put(&qlist[j]->sk);
>> }
>>
>> +/* Determine features to be applied to the packet as it
>
> /*
> * Please use comments
> * like this
> */
>
>> + * gets sent to the tap. The features depend on the offloads
>> + * set by user and by whether or not IFF_VNET_HDR is enabled.
>> + */
>> +static netdev_features_t macvtap_skb_features(struct sk_buff *skb,
>> + struct macvtap_queue *q)
>> +{
>> + struct macvlan_dev *vlan = netdev_priv(skb->dev);
>> + netdev_features_t features = netif_skb_features(skb);
>> +
>> + /* If VNET_HDR is enabled, user can receive offload info so
>> + * use user-specified tap_features.
>> + * Otherwise, mask off all tap offload features.
>> + */
>> + if (q->flags & IFF_VNET_HDR)
>> + features &= vlan->tap_features;
>> + else
>> + features &= ~TUN_OFFLOADS;
>> +
>> + return features;
>> +}
>> +
>
> Okay, but this means we need to do netdev_update_features
> on SETIFF since it changes q->flags.
Why? The whole point it not to change features on SETIFF, so that
they are preserved for when the flags change again. It just that
flag values either allow whatever offload user specified (correctly
masked via netdev_update_features) or turn off all offloads at the tap.
-vlad
>> /*
>> * Forward happens for data that gets sent from one macvlan
>> * endpoint to another one in bridge mode. We just take
>> @@ -276,9 +298,9 @@ static void macvtap_del_queues(struct net_device *dev)
>> */
>> static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
>> {
>> - struct macvlan_dev *vlan = netdev_priv(dev);
>> struct macvtap_queue *q = macvtap_get_queue(dev, skb);
>> netdev_features_t features;
>> +
>> if (!q)
>> goto drop;
>>
>> @@ -289,7 +311,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
>> /* Apply the forward feature mask so that we perform segmentation
>> * according to users wishes.
>> */
>> - features = netif_skb_features(skb) & vlan->tap_features;
>> + features = macvtap_skb_features(skb, q);
>> if (netif_needs_gso(skb, features)) {
>> struct sk_buff *segs = __skb_gso_segment(skb, features, false);
>>
>> @@ -1161,10 +1183,6 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>> TUN_F_TSO_ECN | TUN_F_UFO))
>> return -EINVAL;
>>
>> - /* TODO: only accept frames with the features that
>> - got enabled for forwarded frames */
>> - if (!(q->flags & IFF_VNET_HDR))
>> - return -EINVAL;
>> rtnl_lock();
>> ret = set_offload(q, arg);
>> rtnl_unlock();
>> --
>> 1.8.1.4
^ permalink raw reply
* Re: Specifying netdev name on kernel boot?
From: Ben Greear @ 2013-08-15 18:26 UTC (permalink / raw)
To: Dan Williams; +Cc: netdev
In-Reply-To: <1376590143.2180.11.camel@dcbw.foobar.com>
On 08/15/2013 11:09 AM, Dan Williams wrote:
> On Thu, 2013-08-15 at 08:50 -0700, Ben Greear wrote:
>> It seems they finally broke udev in Fedora 19...it can no longer
>> rename ethX to ethY, for whatever reason.
>
> It's intentional AFAIK, it won't rename devices into the kernel
> namespace (eth, wlan, usb, wwan) because it's inherently race-prone and
> there's no way to know that the kernel didn't just detect a new eth2
> while you're about to rename your eth0 to eth2. There actually have
> been a lot of problems with that in past, because bus enumeration order
> is not guaranteed.
>
> I'm pretty sure you drop a rule in /etc/udev/rules.d to what you want:
>
> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="?*", ATTR{address}=="00:xx:xx:xx:xx:xx", ATTR{dev_id}=="0x0", ATTR{type}=="1", KERNEL=="eth*", NAME="eth0"
This used to work, but no longer does in Fedora 19. If you do NAME="eth0r" then
it does work.
It shouldn't actually race (or at least shouldn't cause problems) at
the kernel level, and user-space should be able to detect a conflict
and take evasive action. Even if the rename itself fails due to a
race, user-space can just try again and it should work after a few tries.
I opened a bug against Fedora udev, for what that is worth.
> Or if you install biosdevname, you'll get the nice BIOS names for the
> interfaces which is usually something like "slot1nic0" or "builtin0" or
> something like that.
>
> (and yeah, one of my devices is "enp0s29f7u4" which is just f-king ugly.
> *I* know what that means and why udev is doing it, but I also sympathize
> with those who don't...)
Yeah, I really want to avoid those types of names.
I hacked up a perl script that does the rename, but it can cause
some churn because I can't find any useful way to make systemd
put the rename logic between udev and network bringup. I am
mostly clueless on systemd, so perhaps it's easier than it looked.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask
From: Michael S. Tsirkin @ 2013-08-15 18:24 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
In-Reply-To: <1376586173-4242-2-git-send-email-vyasevic@redhat.com>
On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
> The features of the macvlan are based on the features of lower
> device and thus can have checksum featurs other then IFF_F_HW_CSUM
s/featurs/features/
:set spell spelllang=en_us
or whatever's the equivalent in your editor of choice.
> set. However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM. Thus
> when performing gso segmentation during macvtap_forward(),
> it is possbile to end up with skbs that have ip_summed set
> to CHECKSUM_PARTIAL. This is incorrect when the user
> turns off checksum offloading.
>
> Include all possible checksum offload values so that
> we'll properly mask them off when performing GSO.
>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
> drivers/net/macvtap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index b51db2a..8121358 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
>
> static const struct proto_ops macvtap_socket_ops;
>
> -#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> +#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> NETIF_F_TSO6 | NETIF_F_UFO)
> #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
> /*
Okay so you are talking about hardware that sets some other
checksum bit besides HW_CSUM, e.g. IP_CSUM, so
vlan->tap_features = vlan->dev->features &
(feature_mask | ~TUN_OFFLOADS);
will not clear IP_CSUM even if feature_mask is 0.
Maybe mention this in the changelog, in case user
will wonder whether his hardware is affected.
So I agree, that's a bug, but if you make this change the reverse will hold
(on this hardware):
if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
so checksum offloading won't work.
So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
and not just in this place.
Also - Cc stable?
> --
> 1.8.1.4
^ permalink raw reply
* Re: Specifying netdev name on kernel boot?
From: Dan Williams @ 2013-08-15 18:09 UTC (permalink / raw)
To: Ben Greear; +Cc: netdev
In-Reply-To: <520CF8BB.2080702@candelatech.com>
On Thu, 2013-08-15 at 08:50 -0700, Ben Greear wrote:
> It seems they finally broke udev in Fedora 19...it can no longer
> rename ethX to ethY, for whatever reason.
It's intentional AFAIK, it won't rename devices into the kernel
namespace (eth, wlan, usb, wwan) because it's inherently race-prone and
there's no way to know that the kernel didn't just detect a new eth2
while you're about to rename your eth0 to eth2. There actually have
been a lot of problems with that in past, because bus enumeration order
is not guaranteed.
I'm pretty sure you drop a rule in /etc/udev/rules.d to what you want:
SUBSYSTEM=="net", ACTION=="add", DRIVERS=="?*", ATTR{address}=="00:xx:xx:xx:xx:xx", ATTR{dev_id}=="0x0", ATTR{type}=="1", KERNEL=="eth*", NAME="eth0"
Or if you install biosdevname, you'll get the nice BIOS names for the
interfaces which is usually something like "slot1nic0" or "builtin0" or
something like that.
(and yeah, one of my devices is "enp0s29f7u4" which is just f-king ugly.
*I* know what that means and why udev is doing it, but I also sympathize
with those who don't...)
Dan
> Is there a way to specify the naming on the kernel command line?
>
> I found something in google that made me think this might work:
>
> ifname=eth0:00:30:48:fc:17:a2 ifname=eth1:00:30:48:fc:17:a3 ifname=eth2:00:e0:ed:1c:ec:e4 ifname=eth3:00:e0:ed:1c:ec:e5
>
> but it doesn't seem to have any affect.
>
> I'm using ixgbe and e1000e drivers, loaded as modules, in case that matters.
>
> Thanks,
> Ben
>
^ permalink raw reply
* [PATCH] macvtap: fix up direction in comment on offloading
From: Michael S. Tsirkin @ 2013-08-15 17:46 UTC (permalink / raw)
To: linux-kernel
Cc: David S. Miller, Jason Wang, Michael S. Tsirkin, Eric Dumazet,
Vlad Yasevich, netdev
It speaks about receiving frames, so while
it says GSO, it really means GRO.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/net/macvtap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index a98fb0e..a98ed9f 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -1047,7 +1047,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
* accept TSO frames and turning it off means that user space
* does not support TSO.
* For macvtap, we have to invert it to mean the same thing.
- * When user space turns off TSO, we turn off GSO/LRO so that
+ * When user space turns off TSO, we turn off GRO/LRO so that
* user-space will not receive TSO frames.
*/
if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
--
MST
^ permalink raw reply related
* Re: [PATCH 1/3 v4] ipv6: do not disable temp_address when reaching max_address
From: Hannes Frederic Sowa @ 2013-08-15 17:36 UTC (permalink / raw)
To: Ding Tianhong
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Jon Maloy, Eric Dumazet,
Netdev, kargig, ppandit
In-Reply-To: <520C3A13.3060106@huawei.com>
On Thu, Aug 15, 2013 at 10:16:51AM +0800, Ding Tianhong wrote:
> I am afraid that if remove the max limit from the ipv6_create_tempaddr, the tool flood_route26 attack will create huge address to the temp_list, it will be a huge list,
> may it destroy something or not?
I just tested, no it does not. Because it will only create a temporary address
for each prefix received, which still is limited by max_addresses. But of
course, more review is needed here. Maybe there is still a possibility to DoS?
Now we have to check why these addresses don't go out of tentative state.
Greetings,
Hannes
^ permalink raw reply
* Re: [PATCH net] net: tg3: fix NULL pointer dereference in tg3_io_error_detected and tg3_io_slot_reset
From: Nithin Nayak Sujir @ 2013-08-15 17:32 UTC (permalink / raw)
To: David Miller; +Cc: netdev, dborkman, shangw, mchan
In-Reply-To: <20130815.010726.719593168454341579.davem@davemloft.net>
>> Cc: Gavin Shan <shangw@linux.vnet.ibm.com>
>> Cc: Michael Chan <mchan@broadcom.com>
>> Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com>
>
> Can I get some reviews from the Broadcom folks?
>
David,
I made the additional changes to Daniel's patch and Michael Chan reviewed it
before I submitted.
Sorry, I forgot to add Michael's Signed-off-by. Do you want me to resend it with
his Signed-off-by added?
Thanks,
Nithin.
> Thanks.
>
^ permalink raw reply
* Re: [PATCH v3 2/2] macvtap: Allow tap features change when IFF_VNET_HDR is disabled.
From: Michael S. Tsirkin @ 2013-08-15 17:33 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
In-Reply-To: <1376586173-4242-3-git-send-email-vyasevic@redhat.com>
On Thu, Aug 15, 2013 at 01:02:53PM -0400, Vlad Yasevich wrote:
> When the user turns off IFF_VNET_HDR flag, attempts to change
> offload features via TUNSETOFFLOAD do not work. This could cause
> GSO packets to be delivered to the user when the user is
> not prepared to handle them.
>
> To solve, allow processing of TUNSETOFFLOAD when IFF_VNET_HDR is
> disabled. Also, take the setting of IFF_VNET_HDR into consideration
> when determining the feature set to apply to incommig traffic.
> If IFF_VNET_HDR is set, we use user-specfied feature set. if the
> IFF_VNET_HDR is clear, we mask off all tun-specific offload features,
> since user can not be notified of the possbile offloads.
>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
> drivers/net/macvtap.c | 30 ++++++++++++++++++++++++------
> 1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index 8121358..df45a59 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -269,6 +269,28 @@ static void macvtap_del_queues(struct net_device *dev)
> sock_put(&qlist[j]->sk);
> }
>
> +/* Determine features to be applied to the packet as it
/*
* Please use comments
* like this
*/
> + * gets sent to the tap. The features depend on the offloads
> + * set by user and by whether or not IFF_VNET_HDR is enabled.
> + */
> +static netdev_features_t macvtap_skb_features(struct sk_buff *skb,
> + struct macvtap_queue *q)
> +{
> + struct macvlan_dev *vlan = netdev_priv(skb->dev);
> + netdev_features_t features = netif_skb_features(skb);
> +
> + /* If VNET_HDR is enabled, user can receive offload info so
> + * use user-specified tap_features.
> + * Otherwise, mask off all tap offload features.
> + */
> + if (q->flags & IFF_VNET_HDR)
> + features &= vlan->tap_features;
> + else
> + features &= ~TUN_OFFLOADS;
> +
> + return features;
> +}
> +
Okay, but this means we need to do netdev_update_features
on SETIFF since it changes q->flags.
> /*
> * Forward happens for data that gets sent from one macvlan
> * endpoint to another one in bridge mode. We just take
> @@ -276,9 +298,9 @@ static void macvtap_del_queues(struct net_device *dev)
> */
> static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
> {
> - struct macvlan_dev *vlan = netdev_priv(dev);
> struct macvtap_queue *q = macvtap_get_queue(dev, skb);
> netdev_features_t features;
> +
> if (!q)
> goto drop;
>
> @@ -289,7 +311,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
> /* Apply the forward feature mask so that we perform segmentation
> * according to users wishes.
> */
> - features = netif_skb_features(skb) & vlan->tap_features;
> + features = macvtap_skb_features(skb, q);
> if (netif_needs_gso(skb, features)) {
> struct sk_buff *segs = __skb_gso_segment(skb, features, false);
>
> @@ -1161,10 +1183,6 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
> TUN_F_TSO_ECN | TUN_F_UFO))
> return -EINVAL;
>
> - /* TODO: only accept frames with the features that
> - got enabled for forwarded frames */
> - if (!(q->flags & IFF_VNET_HDR))
> - return -EINVAL;
> rtnl_lock();
> ret = set_offload(q, arg);
> rtnl_unlock();
> --
> 1.8.1.4
^ permalink raw reply
* Re: [PATCH 1/7] uio: add module owner to prevent inappropriate module unloading
From: Greg Kroah-Hartman @ 2013-08-15 17:13 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Benedikt Spranger, netdev, Alexander Frank, Hans J. Koch,
Holger Dengler
In-Reply-To: <520D07D6.70407@linutronix.de>
On Thu, Aug 15, 2013 at 06:54:46PM +0200, Sebastian Andrzej Siewior wrote:
> On 08/15/2013 06:42 PM, Greg Kroah-Hartman wrote:
> >> For this to happen you would need a refcount in uio-core which learns
> >> that the device is gone and does not invoke any callbacks because the
> >> device is gone. Something like you have in USB where you return 0 on
> >> reads from ttyUSB after someone pulled the cable.
> >
> > That happens because we invalidate the filehandle in the tty layer by
> > tearing everything down in the usb serial driver. And yes, uio also
> > needs to do the same thing, if it doesn't already.
>
> Ah. Do you have a handy pointer where / how you do this?
The tty layer isn't exactly "handy" :)
Somewhere we send a HANGUP signal to userspace, and I think we
invalidate the file handle somehow, it's been years since I last looked
at that code, sorry.
greg k-h
^ permalink raw reply
* [PATCH v3 2/2] macvtap: Allow tap features change when IFF_VNET_HDR is disabled.
From: Vlad Yasevich @ 2013-08-15 17:02 UTC (permalink / raw)
To: netdev; +Cc: mst, Vlad Yasevich
In-Reply-To: <1376586173-4242-1-git-send-email-vyasevic@redhat.com>
When the user turns off IFF_VNET_HDR flag, attempts to change
offload features via TUNSETOFFLOAD do not work. This could cause
GSO packets to be delivered to the user when the user is
not prepared to handle them.
To solve, allow processing of TUNSETOFFLOAD when IFF_VNET_HDR is
disabled. Also, take the setting of IFF_VNET_HDR into consideration
when determining the feature set to apply to incommig traffic.
If IFF_VNET_HDR is set, we use user-specfied feature set. if the
IFF_VNET_HDR is clear, we mask off all tun-specific offload features,
since user can not be notified of the possbile offloads.
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
drivers/net/macvtap.c | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 8121358..df45a59 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -269,6 +269,28 @@ static void macvtap_del_queues(struct net_device *dev)
sock_put(&qlist[j]->sk);
}
+/* Determine features to be applied to the packet as it
+ * gets sent to the tap. The features depend on the offloads
+ * set by user and by whether or not IFF_VNET_HDR is enabled.
+ */
+static netdev_features_t macvtap_skb_features(struct sk_buff *skb,
+ struct macvtap_queue *q)
+{
+ struct macvlan_dev *vlan = netdev_priv(skb->dev);
+ netdev_features_t features = netif_skb_features(skb);
+
+ /* If VNET_HDR is enabled, user can receive offload info so
+ * use user-specified tap_features.
+ * Otherwise, mask off all tap offload features.
+ */
+ if (q->flags & IFF_VNET_HDR)
+ features &= vlan->tap_features;
+ else
+ features &= ~TUN_OFFLOADS;
+
+ return features;
+}
+
/*
* Forward happens for data that gets sent from one macvlan
* endpoint to another one in bridge mode. We just take
@@ -276,9 +298,9 @@ static void macvtap_del_queues(struct net_device *dev)
*/
static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
{
- struct macvlan_dev *vlan = netdev_priv(dev);
struct macvtap_queue *q = macvtap_get_queue(dev, skb);
netdev_features_t features;
+
if (!q)
goto drop;
@@ -289,7 +311,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
/* Apply the forward feature mask so that we perform segmentation
* according to users wishes.
*/
- features = netif_skb_features(skb) & vlan->tap_features;
+ features = macvtap_skb_features(skb, q);
if (netif_needs_gso(skb, features)) {
struct sk_buff *segs = __skb_gso_segment(skb, features, false);
@@ -1161,10 +1183,6 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
TUN_F_TSO_ECN | TUN_F_UFO))
return -EINVAL;
- /* TODO: only accept frames with the features that
- got enabled for forwarded frames */
- if (!(q->flags & IFF_VNET_HDR))
- return -EINVAL;
rtnl_lock();
ret = set_offload(q, arg);
rtnl_unlock();
--
1.8.1.4
^ permalink raw reply related
* [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask
From: Vlad Yasevich @ 2013-08-15 17:02 UTC (permalink / raw)
To: netdev; +Cc: mst, Vlad Yasevich
In-Reply-To: <1376586173-4242-1-git-send-email-vyasevic@redhat.com>
The features of the macvlan are based on the features of lower
device and thus can have checksum featurs other then IFF_F_HW_CSUM
set. However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM. Thus
when performing gso segmentation during macvtap_forward(),
it is possbile to end up with skbs that have ip_summed set
to CHECKSUM_PARTIAL. This is incorrect when the user
turns off checksum offloading.
Include all possible checksum offload values so that
we'll properly mask them off when performing GSO.
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
drivers/net/macvtap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index b51db2a..8121358 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
static const struct proto_ops macvtap_socket_ops;
-#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
+#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
NETIF_F_TSO6 | NETIF_F_UFO)
#define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
/*
--
1.8.1.4
^ permalink raw reply related
* [PATCH v3 0/2] Correctly perform offloads when VNET_HDR is disabled
From: Vlad Yasevich @ 2013-08-15 17:02 UTC (permalink / raw)
To: netdev; +Cc: mst, Vlad Yasevich
This is v3 of the patch. It as been spit into 2 sperate patches as requested.
The asymetry of behavior has been removed. Now it doesn't matter which
order the VNET_HDR and TUNSETOFFLOAD are performed. The TUNSETOFFLOAD
ioctl always modifies the ioctl, and IFF_VNET_HDR can be turned on and off
independed of that. What is different is that when computing features for
GSO, the value of IFF_VNET_HDR is also consulted and a different mask is used.
This way, the following sequence:
clear IFF_VNET_HDR
change TUNSETOFFLOAD
set IFF_VNET_HDR
works and packet packets are delivered in correct format at all times.
Vlad Yasevich (2):
macvtap: include all checksum offloads in TUN_OFFLOAD mask
macvtap: Correctly set tap features when IFF_VNET_HDR is disabled.
drivers/net/macvtap.c | 32 +++++++++++++++++++++++++-------
1 file changed, 25 insertions(+), 7 deletions(-)
--
1.8.1.4
^ permalink raw reply
* Re: [PATCH 1/7] uio: add module owner to prevent inappropriate module unloading
From: Sebastian Andrzej Siewior @ 2013-08-15 16:54 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Benedikt Spranger, netdev, Alexander Frank, Hans J. Koch,
Holger Dengler
In-Reply-To: <20130815164227.GC15688@kroah.com>
On 08/15/2013 06:42 PM, Greg Kroah-Hartman wrote:
>> For this to happen you would need a refcount in uio-core which learns
>> that the device is gone and does not invoke any callbacks because the
>> device is gone. Something like you have in USB where you return 0 on
>> reads from ttyUSB after someone pulled the cable.
>
> That happens because we invalidate the filehandle in the tty layer by
> tearing everything down in the usb serial driver. And yes, uio also
> needs to do the same thing, if it doesn't already.
Ah. Do you have a handy pointer where / how you do this?
>
> thanks,
>
> greg k-h
Sebastian
^ permalink raw reply
* Re: [PATCH 1/7] uio: add module owner to prevent inappropriate module unloading
From: Greg Kroah-Hartman @ 2013-08-15 16:42 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Benedikt Spranger, netdev, Alexander Frank, Hans J. Koch,
Holger Dengler
In-Reply-To: <520CFBE0.5070006@linutronix.de>
On Thu, Aug 15, 2013 at 06:03:44PM +0200, Sebastian Andrzej Siewior wrote:
> On 08/15/2013 05:55 PM, Greg Kroah-Hartman wrote:
> > But that's a "platform" device, for a resource that is described as not
> > going away.
> >
> > If this is really a mfd device, then make your uio driver a mfd driver,
> > not a platform driver for a resource that isn't under your control.
>
> As you described it later yourself: You have the same problem if you
> manually unbind the platform_device from the driver while the device
> node is open.
No, at that point in time the remove function of the uio driver should
be called, and you can invalidate everything then. Or the driver should
be doing that, odds are, it needs to be fixed because no one checks for
that :)
> >> If you look now at uio_write() then you will notice that it will
> >> deference idev->info->irqcontrol but once the device is gone the memory
> >> starting at info is gone, not to mention the code behind irqcontrol.
> >
> > It sounds like the wrong uio driver is binding to this device, fix the
> > uio driver and you should be fine, right?
>
> For this to happen you would need a refcount in uio-core which learns
> that the device is gone and does not invoke any callbacks because the
> device is gone. Something like you have in USB where you return 0 on
> reads from ttyUSB after someone pulled the cable.
That happens because we invalidate the filehandle in the tty layer by
tearing everything down in the usb serial driver. And yes, uio also
needs to do the same thing, if it doesn't already.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] net: rfkill: Do not ignore errors from regulator_enable()
From: Johannes Berg @ 2013-08-15 16:17 UTC (permalink / raw)
To: Luis Henriques
Cc: linux-kernel, netdev, linux-wireless, David S. Miller,
John W. Linville
In-Reply-To: <1376518206-10710-1-git-send-email-luis.henriques@canonical.com>
On Wed, 2013-08-14 at 23:10 +0100, Luis Henriques wrote:
> Function regulator_enable() may return an error that has to be checked.
> This patch changes function rfkill_regulator_set_block() so that it checks
> for the return code. Also, rfkill_data->reg_enabled is set to 'true' only
> if there is no error.
Applied.
johannes
^ permalink raw reply
* Re: [PATCH 1/7] uio: add module owner to prevent inappropriate module unloading
From: Sebastian Andrzej Siewior @ 2013-08-15 16:03 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Benedikt Spranger, netdev, Alexander Frank, Hans J. Koch,
Holger Dengler
In-Reply-To: <20130815155508.GA14792@kroah.com>
On 08/15/2013 05:55 PM, Greg Kroah-Hartman wrote:
> But that's a "platform" device, for a resource that is described as not
> going away.
>
> If this is really a mfd device, then make your uio driver a mfd driver,
> not a platform driver for a resource that isn't under your control.
As you described it later yourself: You have the same problem if you
manually unbind the platform_device from the driver while the device
node is open.
>> If you look now at uio_write() then you will notice that it will
>> deference idev->info->irqcontrol but once the device is gone the memory
>> starting at info is gone, not to mention the code behind irqcontrol.
>
> It sounds like the wrong uio driver is binding to this device, fix the
> uio driver and you should be fine, right?
For this to happen you would need a refcount in uio-core which learns
that the device is gone and does not invoke any callbacks because the
device is gone. Something like you have in USB where you return 0 on
reads from ttyUSB after someone pulled the cable.
> A module reference count will not "save" you from a device going away,
> only a code chunk going away. That is why no other subsystem has this
> type of thing. If you dynamically remove the mfd device, but not remove
> the module (i.e. through the sysfs files to do that), then you would
> still have this same problem, right?
Yes, I think so.
> There's a reason the driver core doesn't deal with module reference
> counts, it's not the proper thing for devices. So I'm not willing to
> add it to the UIO code either, as it's not the correct thing for it.
okay.
>
> thanks,
>
> greg k-h
Sebastian
^ permalink raw reply
* Re: [PATCH 1/7] uio: add module owner to prevent inappropriate module unloading
From: Greg Kroah-Hartman @ 2013-08-15 15:55 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Benedikt Spranger, netdev, Alexander Frank, Hans J. Koch,
Holger Dengler
In-Reply-To: <520C8EC9.6040603@linutronix.de>
On Thu, Aug 15, 2013 at 10:18:17AM +0200, Sebastian Andrzej Siewior wrote:
> On 08/15/2013 10:09 AM, Greg Kroah-Hartman wrote:
> > Do you have a specific example of an in-tree UIO driver that has this
> > problem that I can look at to try to understand this better?
>
> grep for "uio_pdrv" and you find a few devices in arm and sh tree. Each
> one is created once at boot time and never removed. With mfd the device
> can be removed.
But that's a "platform" device, for a resource that is described as not
going away.
If this is really a mfd device, then make your uio driver a mfd driver,
not a platform driver for a resource that isn't under your control.
> If you look now at uio_write() then you will notice that it will
> deference idev->info->irqcontrol but once the device is gone the memory
> starting at info is gone, not to mention the code behind irqcontrol.
It sounds like the wrong uio driver is binding to this device, fix the
uio driver and you should be fine, right?
A module reference count will not "save" you from a device going away,
only a code chunk going away. That is why no other subsystem has this
type of thing. If you dynamically remove the mfd device, but not remove
the module (i.e. through the sysfs files to do that), then you would
still have this same problem, right?
There's a reason the driver core doesn't deal with module reference
counts, it's not the proper thing for devices. So I'm not willing to
add it to the UIO code either, as it's not the correct thing for it.
thanks,
greg k-h
^ permalink raw reply
* Specifying netdev name on kernel boot?
From: Ben Greear @ 2013-08-15 15:50 UTC (permalink / raw)
To: netdev
It seems they finally broke udev in Fedora 19...it can no longer
rename ethX to ethY, for whatever reason.
Is there a way to specify the naming on the kernel command line?
I found something in google that made me think this might work:
ifname=eth0:00:30:48:fc:17:a2 ifname=eth1:00:30:48:fc:17:a3 ifname=eth2:00:e0:ed:1c:ec:e4 ifname=eth3:00:e0:ed:1c:ec:e5
but it doesn't seem to have any affect.
I'm using ixgbe and e1000e drivers, loaded as modules, in case that matters.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH net] skge: dma_sync the whole receive buffer
From: Stephen Hemminger @ 2013-08-15 15:41 UTC (permalink / raw)
To: poma; +Cc: David Miller, netdev
In-Reply-To: <520BCC72.8040002@gmail.com>
On Wed, 14 Aug 2013 20:29:06 +0200
poma <pomidorabelisima@gmail.com> wrote:
> On 14.08.2013 18:20, Stephen Hemminger wrote:
> > On Wed, 14 Aug 2013 12:20:03 +0200
> > poma <pomidorabelisima@gmail.com> wrote:
> >
> >> On 14.08.2013 03:00, Stephen Hemminger wrote:
> >>> On Tue, 13 Aug 2013 15:09:55 -0700 (PDT)
> >>> David Miller <davem@davemloft.net> wrote:
> >>>
> >>>> From: Stephen Hemminger <stephen@networkplumber.org>
> >>>> Date: Sat, 10 Aug 2013 15:02:07 -0700
> >>>>
> >>>>> The DMA sync should sync the whole receive buffer, not just
> >>>>> part of it. Fixes log messages dma_sync_check.
> >>>>>
> >>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >>>>
> >>>> Applied, but I really suspect that your "check DMA mapping errors"
> >>>> patch has added a serious regression. A regression much worse than
> >>>> the bug you were trying to fix with that change.
> >>>
> >>> Argh. The problem is deeper than that. Device got broken somewhere between
> >>> 3.2 and 3.4. My old Dlink card works on 3.2 but gets DMA errors on 3.4.
> >>> The config's are different though so checking that as well.
> >>>
> >>
> >> Can I help you with debugging?
> >> DGE-530T is rather solid device.
> >
> > Don't think it is a hardware problem.
> > The failure is when the board access the Receive ring PCI memory area.
> > This region is allocated with pci_alloc_consistent and therefore should
> > be available. Two possible issues are driver math issues, or hardware
> > problems with where the region is located. Some of these cards don't
> > really have full 64 bit PCI support.
> >
> > My board is:
> > 05:01.0 Ethernet controller: D-Link System Inc Gigabit Ethernet Adapter (rev 11)
> > Subsystem: D-Link System Inc DGE-530T Gigabit Ethernet Adapter
> > Flags: bus master, 66MHz, medium devsel, latency 32, IRQ 18
> > Memory at f7d20000 (32-bit, non-prefetchable) [size=16K]
> > I/O ports at c000 [size=256]
> > Expansion ROM at f7d00000 [disabled] [size=128K]
> > Capabilities: [48] Power Management version 2
> > Capabilities: [50] Vital Product Data
> > Kernel driver in use: skge
> >
> >
> > What is your config?
> >
>
> 01:09.0 Ethernet controller: D-Link System Inc Gigabit Ethernet Adapter
> (rev 11)
> Subsystem: D-Link System Inc DGE-530T Gigabit Ethernet Adapter
> Flags: bus master, 66MHz, medium devsel, latency 64, IRQ 19
> Memory at fbffc000 (32-bit, non-prefetchable) [size=16K]
> I/O ports at b400 [size=256]
> [virtual] Expansion ROM at ec000000 [disabled] [size=128K]
> Capabilities: [48] Power Management version 2
> Capabilities: [50] Vital Product Data
> Kernel driver in use: skge
>
>
> poma
>
In the course of debugging this, I moved the card to another slot
and all the problems went away. I suspect either card insertion or more likely
the crap consumer motherboards don't have full PCI support on some slots.
There doesn't seem to be anyway to address this in software.
^ 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