* [PATCH net v2 1/3] net: bcmgenet: fix off-by-one in incrementing read pointer
From: Florian Fainelli @ 2014-10-10 17:51 UTC (permalink / raw)
To: netdev; +Cc: davem, pgynther, jaedon.shin, Florian Fainelli
In-Reply-To: <1412963514-7718-1-git-send-email-f.fainelli@gmail.com>
Commit b629be5c8399d7c423b92135eb43a86c924d1cbc ("net: bcmgenet: check
harder for out of memory conditions") moved the increment of the local
read pointer *before* reading from the hardware descriptor using
dmadesc_get_length_status(), which creates an off-by-one situation.
Fix this by moving again the read_ptr increment after we have read the
hardware descriptor to get both the control block and the read pointer
back in sync.
Fixes: b629be5c8399 ("net: bcmgenet: check harder for out of memory conditions")
Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:
- moved the rxpktprocessed and rx_read_ptr increment after refiling the control
block
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index fff2634b6f34..fdc9ec09e453 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1285,11 +1285,6 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_priv *priv,
cb = &priv->rx_cbs[priv->rx_read_ptr];
skb = cb->skb;
- rxpktprocessed++;
-
- priv->rx_read_ptr++;
- priv->rx_read_ptr &= (priv->num_rx_bds - 1);
-
/* We do not have a backing SKB, so we do not have a
* corresponding DMA mapping for this incoming packet since
* bcmgenet_rx_refill always either has both skb and mapping or
@@ -1404,6 +1399,10 @@ refill:
err = bcmgenet_rx_refill(priv, cb);
if (err)
netif_err(priv, rx_err, dev, "Rx refill failed\n");
+
+ rxpktprocessed++;
+ priv->rx_read_ptr++;
+ priv->rx_read_ptr &= (priv->num_rx_bds - 1);
}
return rxpktprocessed;
--
1.9.1
^ permalink raw reply related
* [PATCH net v2 0/3] net: bcmgenet & systemport fixes
From: Florian Fainelli @ 2014-10-10 17:51 UTC (permalink / raw)
To: netdev; +Cc: davem, pgynther, jaedon.shin, Florian Fainelli
Hi David,
This patch series fixes an off-by-one error introduced during a previous
change, and the two other fixes fix a wake depth imbalance situation for
the Wake-on-LAN interrupt line.
Thanks!
Florian Fainelli (3):
net: bcmgenet: fix off-by-one in incrementing read pointer
net: bcmgenet: avoid unbalanced enable_irq_wake calls
net: systemport: avoid unbalanced enable_irq_wake calls
drivers/net/ethernet/broadcom/bcmsysport.c | 3 ++-
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 9 ++++-----
drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 4 +++-
3 files changed, 9 insertions(+), 7 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH net 1/3] net: bcmgenet: fix off-by-one in incrementing read pointer
From: Florian Fainelli @ 2014-10-10 17:18 UTC (permalink / raw)
To: Petri Gynther; +Cc: netdev, David Miller, jaedon.shin
In-Reply-To: <CAGXr9JEm4Dmo5QYhsv7ydr8NBujtrJ=gJ9kxSFoiBQ-YVQkGBA@mail.gmail.com>
On 10/10/2014 10:17 AM, Petri Gynther wrote:
> On Fri, Oct 10, 2014 at 9:41 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 10/09/2014 11:01 PM, Petri Gynther wrote:
>>> Hi Florian,
>>>
>>> On Thu, Oct 9, 2014 at 6:06 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> Commit b629be5c8399d7c423b92135eb43a86c924d1cbc ("net: bcmgenet: check
>>>> harder for out of memory conditions") moved the increment of the local
>>>> read pointer *before* reading from the hardware descriptor using
>>>> dmadesc_get_length_status(), which creates an off-by-one situation.
>>>>
>>>> Fix this by moving again the read_ptr increment after we have read the
>>>> hardware descriptor to get both the control block and the read pointer
>>>> back in sync.
>>>>
>>>> Fixes: b629be5c8399 ("net: bcmgenet: check harder for out of memory conditions")
>>>> Reported-by: Jaedon Shin <jaedon.shin@gmail.com>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>> drivers/net/ethernet/broadcom/genet/bcmgenet.c | 6 +++---
>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>>> index fff2634b6f34..f1bcebcbba80 100644
>>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>>> @@ -1287,9 +1287,6 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_priv *priv,
>>>>
>>>> rxpktprocessed++;
>>>>
>>>> - priv->rx_read_ptr++;
>>>> - priv->rx_read_ptr &= (priv->num_rx_bds - 1);
>>>> -
>>>
>>> Wouldn't it be better to move the three lines:
>>> rxpktprocessed++;
>>> priv->rx_read_ptr++;
>>> priv->rx_read_ptr &= (priv->num_rx_bds - 1)
>>>
>>> as the last lines of the while-loop, after the CB refill?
>>
>> That's basically what Jaedon did in his first patch, I don't have strong
>> objections in doing that if you think that makes it look clearer.
>>
>
> I feel that this would be cleaner approach. First do the work (or skip
> to refill), then increment necessary variables, and go back to the
> while-loop.
Sounds good, I will re-submit shortly, thanks!
>
>>>
>>> -- Petri
>>>
>>>
>>>> /* We do not have a backing SKB, so we do not have a
>>>> * corresponding DMA mapping for this incoming packet since
>>>> * bcmgenet_rx_refill always either has both skb and mapping or
>>>> @@ -1332,6 +1329,9 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_priv *priv,
>>>> __func__, p_index, priv->rx_c_index,
>>>> priv->rx_read_ptr, dma_length_status);
>>>>
>>>> + priv->rx_read_ptr++;
>>>> + priv->rx_read_ptr &= (priv->num_rx_bds - 1);
>>>> +
>>>> if (unlikely(!(dma_flag & DMA_EOP) || !(dma_flag & DMA_SOP))) {
>>>> netif_err(priv, rx_status, dev,
>>>> "dropping fragmented packet!\n");
>>>> --
>>>> 1.9.1
>>>>
>>
^ permalink raw reply
* Re: [PATCH net 1/3] net: bcmgenet: fix off-by-one in incrementing read pointer
From: Petri Gynther @ 2014-10-10 17:17 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, David Miller, jaedon.shin
In-Reply-To: <54380C29.9010704@gmail.com>
On Fri, Oct 10, 2014 at 9:41 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 10/09/2014 11:01 PM, Petri Gynther wrote:
>> Hi Florian,
>>
>> On Thu, Oct 9, 2014 at 6:06 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> Commit b629be5c8399d7c423b92135eb43a86c924d1cbc ("net: bcmgenet: check
>>> harder for out of memory conditions") moved the increment of the local
>>> read pointer *before* reading from the hardware descriptor using
>>> dmadesc_get_length_status(), which creates an off-by-one situation.
>>>
>>> Fix this by moving again the read_ptr increment after we have read the
>>> hardware descriptor to get both the control block and the read pointer
>>> back in sync.
>>>
>>> Fixes: b629be5c8399 ("net: bcmgenet: check harder for out of memory conditions")
>>> Reported-by: Jaedon Shin <jaedon.shin@gmail.com>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>> drivers/net/ethernet/broadcom/genet/bcmgenet.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>> index fff2634b6f34..f1bcebcbba80 100644
>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>> @@ -1287,9 +1287,6 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_priv *priv,
>>>
>>> rxpktprocessed++;
>>>
>>> - priv->rx_read_ptr++;
>>> - priv->rx_read_ptr &= (priv->num_rx_bds - 1);
>>> -
>>
>> Wouldn't it be better to move the three lines:
>> rxpktprocessed++;
>> priv->rx_read_ptr++;
>> priv->rx_read_ptr &= (priv->num_rx_bds - 1)
>>
>> as the last lines of the while-loop, after the CB refill?
>
> That's basically what Jaedon did in his first patch, I don't have strong
> objections in doing that if you think that makes it look clearer.
>
I feel that this would be cleaner approach. First do the work (or skip
to refill), then increment necessary variables, and go back to the
while-loop.
>>
>> -- Petri
>>
>>
>>> /* We do not have a backing SKB, so we do not have a
>>> * corresponding DMA mapping for this incoming packet since
>>> * bcmgenet_rx_refill always either has both skb and mapping or
>>> @@ -1332,6 +1329,9 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_priv *priv,
>>> __func__, p_index, priv->rx_c_index,
>>> priv->rx_read_ptr, dma_length_status);
>>>
>>> + priv->rx_read_ptr++;
>>> + priv->rx_read_ptr &= (priv->num_rx_bds - 1);
>>> +
>>> if (unlikely(!(dma_flag & DMA_EOP) || !(dma_flag & DMA_SOP))) {
>>> netif_err(priv, rx_status, dev,
>>> "dropping fragmented packet!\n");
>>> --
>>> 1.9.1
>>>
>
^ permalink raw reply
* Re: [PATCH 1/1] Checkpatch: coding style errors in Nvidia ethernet driver
From: Joe Perches @ 2014-10-10 17:04 UTC (permalink / raw)
To: Akshay Sarode; +Cc: netdev, linux-kernel
In-Reply-To: <20141010164515.GA3596@linux.akshay.com>
On Fri, 2014-10-10 at 22:15 +0530, Akshay Sarode wrote:
> On Fri, Oct 10, 2014 at 08:03:07AM -0700, Joe Perches wrote:
> > On Fri, 2014-10-10 at 13:31 +0530, Akshay Sarode wrote:
> > > ERROR: "foo* bar" should be "foo *bar"
> > > ERROR: do not initialise statics to 0 or NULL
> > > CHECK: spinlock_t definition without comment
> > > Signed-off-by: Akshay Sarode <akshaysarode21@gmail.com>
> > []
> > > diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> > []
> > > @@ -911,12 +913,18 @@ enum {
> > []
> > > /*
> > > * Debug output control for tx_timeout
> > > */
> > > -static bool debug_tx_timeout = false;
> > > +enum {
> > > + NV_DEBUG_TX_TIMEOUT_DISABLED,
> > > + NV_DEBUG_TX_TIMEOUT_ENABLED
> > > +};
> > > +
> > > +static bool debug_tx_timeout = NV_DEBUG_TX_TIMEOUT_DISABLED;
> >
> > Adding this enum is not useful.
> >
> Sorry, If I may have not checked the code properly. I am a newbie here and I was hoping to start with checking coding styles.
No worries, welcome.
Starting with fixing a coding style defect or three is
just fine to learn how to compile and test the kernel.
But generally it's more useful to find things that make
the code better, more readable, smaller, faster, etc.
Also if you have some functional defect or enhancement
to implement, that's even better still.
> I'll check again.
Using an enum for a bool isn't very sensible.
true/false exist already.
> Also there are a whole lot of warnings for line over 80 characters.
I wouldn't bother with long line conversions unless
you're doing something else at the same time.
If you want to do them for the practice, please do
them on files in drivers/staging/.
^ permalink raw reply
* [PATCH iproute2] Changed action labels to [action ] format
From: Vadim Kochan @ 2014-10-10 16:51 UTC (permalink / raw)
To: netdev; +Cc: Vadim Kochan
The reasons for this change were:
- different ip utils uses different labels for action: Deleted/delete
- make an action labels to be easy found in logs
ip monitor:
[DELETED] 192.168.2.0/24 dev winbr0 proto kernel scope link src 192.168.2.100 metric 205
[DELETED] default via 192.168.2.1 dev winbr0 metric 205
[DELETED] 5: winbr0 inet 192.168.2.100/24 brd 192.168.2.255 scope global winbr0
valid_lft forever preferred_lft forever
[DELETED] broadcast 192.168.2.255 dev winbr0 table local proto kernel scope link src 192.168.2.100
[DELETED] broadcast 192.168.2.0 dev winbr0 table local proto kernel scope link src 192.168.2.100
[DELETED] local 192.168.2.100 dev winbr0 table local proto kernel scope host src 192.168.2.100
[DELETED] 224.0.0.251 dev winbr0 lladdr XX:XX:XX:XX:XX:XX NOARP
[DELETED] 224.0.0.22 dev winbr0 lladdr XX:XX:XX:XX:XX:XX NOARP
[DELETED] 192.168.2.1 dev winbr0 lladdr XX:XX:XX:XX:XX:XX REACHABLE
tc monitor:
[DELETED] qdisc dsmark 10: dev lo root indices 0x0040 default_index 0x0001 set_tc_index
qdisc cbq 10: dev lo root rate 100Mbit (bounded,isolated) prio no-transmit
class cbq 10:12 dev lo parent 10: rate 100Mbit (bounded) prio 3
[DELETED] qdisc cbq 10: dev lo root rate 100Mbit (bounded,isolated) prio no-transmit
qdisc htb 10: dev lo root r2q 10 default 0 direct_packets_stat 0 direct_qlen 2
class htb 10:12 dev lo root prio 0 rate 100Mbit ceil 100Mbit burst 1600b cburst 1600b
[DELETED] qdisc htb 10: dev lo root r2q 10 default 0 direct_packets_stat 0 direct_qlen 2
qdisc dsmark 20: dev lo root indices 0x0040 default_index 0x0001 set_tc_index
class dsmark 20:12 dev lo parent 20: mask 0xff value 0x02
[DELETED] qdisc pfifo 0: dev lo parent 20: limit 1p
qdisc prio 10: dev lo parent 20: bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
---
bridge/fdb.c | 2 +-
bridge/link.c | 2 +-
ip/ipaddress.c | 4 ++--
ip/ipaddrlabel.c | 2 +-
ip/ipmroute.c | 2 +-
ip/ipneigh.c | 4 ++--
ip/ipnetns.c | 4 ++--
ip/iproute.c | 2 +-
ip/iprule.c | 2 +-
ip/tcp_metrics.c | 2 +-
ip/xfrm_policy.c | 6 +++---
ip/xfrm_state.c | 6 +++---
tc/m_action.c | 6 +++---
tc/tc_class.c | 2 +-
tc/tc_filter.c | 2 +-
tc/tc_qdisc.c | 2 +-
16 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/bridge/fdb.c b/bridge/fdb.c
index a55fac1..26cff05 100644
--- a/bridge/fdb.c
+++ b/bridge/fdb.c
@@ -88,7 +88,7 @@ int print_fdb(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
n->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
if (n->nlmsg_type == RTM_DELNEIGH)
- fprintf(fp, "Deleted ");
+ fprintf(fp, "[DELETED] ");
if (tb[NDA_LLADDR]) {
SPRINT_BUF(b1);
diff --git a/bridge/link.c b/bridge/link.c
index 90d9e7f..6eb5887 100644
--- a/bridge/link.c
+++ b/bridge/link.c
@@ -125,7 +125,7 @@ int print_linkinfo(const struct sockaddr_nl *who,
}
if (n->nlmsg_type == RTM_DELLINK)
- fprintf(fp, "Deleted ");
+ fprintf(fp, "[DELETED] ");
fprintf(fp, "%d: %s ", ifi->ifi_index,
tb[IFLA_IFNAME] ? rta_getattr_str(tb[IFLA_IFNAME]) : "<nil>");
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 45729d8..5c3d5f1 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -481,7 +481,7 @@ int print_linkinfo(const struct sockaddr_nl *who,
}
if (n->nlmsg_type == RTM_DELLINK)
- fprintf(fp, "Deleted ");
+ fprintf(fp, "[DELETED] ");
fprintf(fp, "%d: %s", ifi->ifi_index,
tb[IFLA_IFNAME] ? rta_getattr_str(tb[IFLA_IFNAME]) : "<nil>");
@@ -694,7 +694,7 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
}
if (n->nlmsg_type == RTM_DELADDR)
- fprintf(fp, "Deleted ");
+ fprintf(fp, "[DELETED] ");
if (filter.oneline || filter.flushb)
fprintf(fp, "%u: %s", ifa->ifa_index, ll_index_to_name(ifa->ifa_index));
diff --git a/ip/ipaddrlabel.c b/ip/ipaddrlabel.c
index b34dd8b..1386586 100644
--- a/ip/ipaddrlabel.c
+++ b/ip/ipaddrlabel.c
@@ -71,7 +71,7 @@ int print_addrlabel(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg
parse_rtattr(tb, IFAL_MAX, IFAL_RTA(ifal), len);
if (n->nlmsg_type == RTM_DELADDRLABEL)
- fprintf(fp, "Deleted ");
+ fprintf(fp, "[DELETED] ");
if (tb[IFAL_ADDRESS]) {
fprintf(fp, "prefix %s/%u ",
diff --git a/ip/ipmroute.c b/ip/ipmroute.c
index be93a98..6c6eb75 100644
--- a/ip/ipmroute.c
+++ b/ip/ipmroute.c
@@ -111,7 +111,7 @@ int print_mroute(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
family = r->rtm_family == RTNL_FAMILY_IPMR ? AF_INET : AF_INET6;
if (n->nlmsg_type == RTM_DELROUTE)
- fprintf(fp, "Deleted ");
+ fprintf(fp, "[DELETED] ");
if (tb[RTA_SRC])
len = snprintf(obuf, sizeof(obuf),
diff --git a/ip/ipneigh.c b/ip/ipneigh.c
index 71a4100..a15ae10 100644
--- a/ip/ipneigh.c
+++ b/ip/ipneigh.c
@@ -251,9 +251,9 @@ int print_neigh(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
}
if (n->nlmsg_type == RTM_DELNEIGH)
- fprintf(fp, "delete ");
+ fprintf(fp, "[DELETED] ");
else if (n->nlmsg_type == RTM_GETNEIGH)
- fprintf(fp, "miss ");
+ fprintf(fp, "[MISS] ");
if (tb[NDA_DST]) {
fprintf(fp, "%s ",
format_host(r->ndm_family,
diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 90a496f..d8f217f 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -504,9 +504,9 @@ static int netns_monitor(int argc, char **argv)
(char *)event < &buf[len];
event = (struct inotify_event *)((char *)event + sizeof(*event) + event->len)) {
if (event->mask & IN_CREATE)
- printf("add %s\n", event->name);
+ printf("[NEW] %s\n", event->name);
if (event->mask & IN_DELETE)
- printf("delete %s\n", event->name);
+ printf("[DELETED] %s\n", event->name);
}
}
return 0;
diff --git a/ip/iproute.c b/ip/iproute.c
index d77b1e3..5288a5d 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -331,7 +331,7 @@ int print_route(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
}
if (n->nlmsg_type == RTM_DELROUTE)
- fprintf(fp, "Deleted ");
+ fprintf(fp, "[DELETED] ");
if ((r->rtm_type != RTN_UNICAST || show_details > 0) && !filter.type)
fprintf(fp, "%s ", rtnl_rtntype_n2a(r->rtm_type, b1, sizeof(b1)));
diff --git a/ip/iprule.c b/ip/iprule.c
index 366878e..ff65f58 100644
--- a/ip/iprule.c
+++ b/ip/iprule.c
@@ -76,7 +76,7 @@ int print_rule(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
host_len = 80;
if (n->nlmsg_type == RTM_DELRULE)
- fprintf(fp, "Deleted ");
+ fprintf(fp, "[DELETED] ");
if (tb[FRA_PRIORITY])
fprintf(fp, "%u:\t", *(unsigned*)RTA_DATA(tb[FRA_PRIORITY]));
diff --git a/ip/tcp_metrics.c b/ip/tcp_metrics.c
index bbbb4cc..58d35fb 100644
--- a/ip/tcp_metrics.c
+++ b/ip/tcp_metrics.c
@@ -190,7 +190,7 @@ static int process_msg(const struct sockaddr_nl *who, struct nlmsghdr *n,
}
if (f.cmd & (CMD_DEL | CMD_FLUSH))
- fprintf(fp, "Deleted ");
+ fprintf(fp, "[DELETED] ");
fprintf(fp, "%s",
format_host(family, dlen, &daddr.data, abuf, sizeof(abuf)));
diff --git a/ip/xfrm_policy.c b/ip/xfrm_policy.c
index 2337d35..3547abe 100644
--- a/ip/xfrm_policy.c
+++ b/ip/xfrm_policy.c
@@ -519,11 +519,11 @@ int xfrm_policy_print(const struct sockaddr_nl *who, struct nlmsghdr *n,
return 0;
if (n->nlmsg_type == XFRM_MSG_DELPOLICY)
- fprintf(fp, "Deleted ");
+ fprintf(fp, "[DELETED] ");
else if (n->nlmsg_type == XFRM_MSG_UPDPOLICY)
- fprintf(fp, "Updated ");
+ fprintf(fp, "[UPDATED] ");
else if (n->nlmsg_type == XFRM_MSG_POLEXPIRE)
- fprintf(fp, "Expired ");
+ fprintf(fp, "[EXPIRED] ");
if (n->nlmsg_type == XFRM_MSG_DELPOLICY) {
//xfrm_policy_id_print();
diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
index fe7708e..4748292 100644
--- a/ip/xfrm_state.c
+++ b/ip/xfrm_state.c
@@ -864,11 +864,11 @@ int xfrm_state_print(const struct sockaddr_nl *who, struct nlmsghdr *n,
return 0;
if (n->nlmsg_type == XFRM_MSG_DELSA)
- fprintf(fp, "Deleted ");
+ fprintf(fp, "[DELETED] ");
else if (n->nlmsg_type == XFRM_MSG_UPDSA)
- fprintf(fp, "Updated ");
+ fprintf(fp, "[UPDATED] ");
else if (n->nlmsg_type == XFRM_MSG_EXPIRE)
- fprintf(fp, "Expired ");
+ fprintf(fp, "[EXPIRED] ");
if (n->nlmsg_type == XFRM_MSG_DELSA)
rta = XFRMSID_RTA(xsid);
diff --git a/tc/m_action.c b/tc/m_action.c
index 486123e..0e8cc74 100644
--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -362,15 +362,15 @@ int print_action(const struct sockaddr_nl *who,
if (n->nlmsg_type == RTM_DELACTION) {
if (n->nlmsg_flags & NLM_F_ROOT) {
- fprintf(fp, "Flushed table ");
+ fprintf(fp, "[FLUSHED] table ");
tab_flush = 1;
} else {
- fprintf(fp, "deleted action ");
+ fprintf(fp, "[DELETED] action ");
}
}
if (n->nlmsg_type == RTM_NEWACTION)
- fprintf(fp, "Added action ");
+ fprintf(fp, "[NEW] action ");
tc_print_action(fp, tb[TCA_ACT_TAB]);
return 0;
diff --git a/tc/tc_class.c b/tc/tc_class.c
index e56bf07..66c989b 100644
--- a/tc/tc_class.c
+++ b/tc/tc_class.c
@@ -182,7 +182,7 @@ int print_class(const struct sockaddr_nl *who,
}
if (n->nlmsg_type == RTM_DELTCLASS)
- fprintf(fp, "deleted ");
+ fprintf(fp, "[DELETED] ");
abuf[0] = 0;
if (t->tcm_handle) {
diff --git a/tc/tc_filter.c b/tc/tc_filter.c
index c3f2d5f..aee1f53 100644
--- a/tc/tc_filter.c
+++ b/tc/tc_filter.c
@@ -211,7 +211,7 @@ int print_filter(const struct sockaddr_nl *who,
}
if (n->nlmsg_type == RTM_DELTFILTER)
- fprintf(fp, "deleted ");
+ fprintf(fp, "[DELETED] ");
fprintf(fp, "filter ");
if (!filter_ifindex || filter_ifindex != t->tcm_ifindex)
diff --git a/tc/tc_qdisc.c b/tc/tc_qdisc.c
index e304858..193e720 100644
--- a/tc/tc_qdisc.c
+++ b/tc/tc_qdisc.c
@@ -228,7 +228,7 @@ int print_qdisc(const struct sockaddr_nl *who,
}
if (n->nlmsg_type == RTM_DELQDISC)
- fprintf(fp, "deleted ");
+ fprintf(fp, "[DELETED] ");
fprintf(fp, "qdisc %s %x: ", rta_getattr_str(tb[TCA_KIND]), t->tcm_handle>>16);
if (filter_ifindex == 0)
--
2.1.0
^ permalink raw reply related
* Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports
From: Alexander Duyck @ 2014-10-10 16:50 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Laight, David Miller, alexander.duyck@gmail.com,
netdev@vger.kernel.org
In-Reply-To: <1412954973.9362.11.camel@edumazet-glaptop2.roam.corp.google.com>
On 10/10/2014 08:29 AM, Eric Dumazet wrote:
> On Fri, 2014-10-10 at 08:14 -0700, Alexander Duyck wrote:
>
>> I think you might be coming to this a little late. The igb and ixgbe
>> drivers had been working this way for a long time. We did a memcpy to
>> move the headers from the page and into the skb->data at an aligned
>> offset. In order to determine the length to memcpy we had a function
>> that could walk through the DMA aligned data to get the header length.
>> The function for that was replaced with the __skb_flow_dissect as it was
>> considered a duplication of code with the flow_dissection functions.
>> However that is obviously not the case now that we are hitting these
>> alignment issues.
>>
>> The question I have in all this is do I push forward and make
>> __skb_flow_dissect work with unaligned accesses, or do I back off and
>> put something equivilent to igb/ixgbe_get_headlen functions in the
>> kernel in order to deal with the unaligned accesses as they had no
>> issues with them since they were only concerned with getting the header
>> length and kept all accesses 16b aligned.
>>
> I see nothing wrong dealing with unaligned accesses, as these helpers
> are nop on x86 or CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y arches.
Still it means possibly hurting performance on those platforms that
don't have it defined.
If I just use get_unaligned that is pretty easy in terms of cleanup for
the ports and IPv4 addresses, the IPv6 will still be a significant
hurdle to overcome though.
Thanks,
Alex
^ permalink raw reply
* Re: [PATCH 1/1] Checkpatch: coding style errors in Nvidia ethernet driver
From: Akshay Sarode @ 2014-10-10 16:45 UTC (permalink / raw)
To: Joe Perches; +Cc: netdev, linux-kernel
In-Reply-To: <1412953387.8770.23.camel@joe-AO725>
On Fri, Oct 10, 2014 at 08:03:07AM -0700, Joe Perches wrote:
> On Fri, 2014-10-10 at 13:31 +0530, Akshay Sarode wrote:
> > ERROR: "foo* bar" should be "foo *bar"
> > ERROR: do not initialise statics to 0 or NULL
> > CHECK: spinlock_t definition without comment
> > Signed-off-by: Akshay Sarode <akshaysarode21@gmail.com>
> []
> > diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> []
> > @@ -911,12 +913,18 @@ enum {
> []
> > /*
> > * Debug output control for tx_timeout
> > */
> > -static bool debug_tx_timeout = false;
> > +enum {
> > + NV_DEBUG_TX_TIMEOUT_DISABLED,
> > + NV_DEBUG_TX_TIMEOUT_ENABLED
> > +};
> > +
> > +static bool debug_tx_timeout = NV_DEBUG_TX_TIMEOUT_DISABLED;
>
> Adding this enum is not useful.
>
Sorry, If I may have not checked the code properly. I am a newbie here and I was hoping to start with checking coding styles.
I'll check again. Also there are a whole lot of warnings for line over 80 characters.
Regards,
Akshay
^ permalink raw reply
* Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports
From: David Miller @ 2014-10-10 16:41 UTC (permalink / raw)
To: eric.dumazet; +Cc: David.Laight, alexander.h.duyck, alexander.duyck, netdev
In-Reply-To: <1412955219.9362.13.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 10 Oct 2014 08:33:39 -0700
> On Fri, 2014-10-10 at 14:57 +0000, David Laight wrote:
>
>> I think there is code to copy the IP and TCP headers to aligned memory
>> before they are parsed.
>
> There is no such thing. You are here on netdev list, please read the
> code before doing such claims.
+1
^ permalink raw reply
* Re: [PATCH net 1/3] net: bcmgenet: fix off-by-one in incrementing read pointer
From: Florian Fainelli @ 2014-10-10 16:41 UTC (permalink / raw)
To: Petri Gynther; +Cc: netdev, David Miller, jaedon.shin
In-Reply-To: <CAGXr9JHBVW9rOiSUCCsVnYp73OPdyynP35xiAnKjRsVCf8fK9Q@mail.gmail.com>
On 10/09/2014 11:01 PM, Petri Gynther wrote:
> Hi Florian,
>
> On Thu, Oct 9, 2014 at 6:06 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> Commit b629be5c8399d7c423b92135eb43a86c924d1cbc ("net: bcmgenet: check
>> harder for out of memory conditions") moved the increment of the local
>> read pointer *before* reading from the hardware descriptor using
>> dmadesc_get_length_status(), which creates an off-by-one situation.
>>
>> Fix this by moving again the read_ptr increment after we have read the
>> hardware descriptor to get both the control block and the read pointer
>> back in sync.
>>
>> Fixes: b629be5c8399 ("net: bcmgenet: check harder for out of memory conditions")
>> Reported-by: Jaedon Shin <jaedon.shin@gmail.com>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> drivers/net/ethernet/broadcom/genet/bcmgenet.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index fff2634b6f34..f1bcebcbba80 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -1287,9 +1287,6 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_priv *priv,
>>
>> rxpktprocessed++;
>>
>> - priv->rx_read_ptr++;
>> - priv->rx_read_ptr &= (priv->num_rx_bds - 1);
>> -
>
> Wouldn't it be better to move the three lines:
> rxpktprocessed++;
> priv->rx_read_ptr++;
> priv->rx_read_ptr &= (priv->num_rx_bds - 1)
>
> as the last lines of the while-loop, after the CB refill?
That's basically what Jaedon did in his first patch, I don't have strong
objections in doing that if you think that makes it look clearer.
>
> -- Petri
>
>
>> /* We do not have a backing SKB, so we do not have a
>> * corresponding DMA mapping for this incoming packet since
>> * bcmgenet_rx_refill always either has both skb and mapping or
>> @@ -1332,6 +1329,9 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_priv *priv,
>> __func__, p_index, priv->rx_c_index,
>> priv->rx_read_ptr, dma_length_status);
>>
>> + priv->rx_read_ptr++;
>> + priv->rx_read_ptr &= (priv->num_rx_bds - 1);
>> +
>> if (unlikely(!(dma_flag & DMA_EOP) || !(dma_flag & DMA_SOP))) {
>> netif_err(priv, rx_status, dev,
>> "dropping fragmented packet!\n");
>> --
>> 1.9.1
>>
^ permalink raw reply
* Re: [PATCH v2] net/phy: micrel: Add clock support for KSZ8021/KSZ8031
From: Florian Fainelli @ 2014-10-10 16:38 UTC (permalink / raw)
To: Sascha Hauer; +Cc: netdev, linux-kernel, kernel
In-Reply-To: <1412927285-6809-1-git-send-email-s.hauer@pengutronix.de>
On 10/10/2014 12:48 AM, Sascha Hauer wrote:
> The KSZ8021 and KSZ8031 support RMII reference input clocks of 25MHz
> and 50MHz. Both PHYs differ in the default frequency they expect
> after reset. If this differs from the actual input clock, then
> register 0x1f bit 7 must be changed.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>
> Changes since v1:
>
> - Move clock handling to the probe callback
> - Bail out with an error for invalid clock rates
>
> Documentation/devicetree/bindings/net/micrel.txt | 6 +++++
> drivers/net/phy/micrel.c | 31 ++++++++++++++++++++++--
> include/linux/micrel_phy.h | 1 +
> 3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
> index 98a3e61..e1d99b9 100644
> --- a/Documentation/devicetree/bindings/net/micrel.txt
> +++ b/Documentation/devicetree/bindings/net/micrel.txt
> @@ -16,3 +16,9 @@ Optional properties:
> KSZ8051: register 0x1f, bits 5..4
>
> See the respective PHY datasheet for the mode values.
> +
> + - clocks, clock-names: contains clocks according to the common clock bindings.
> +
> + supported clocks:
> + - KSZ8021, KSZ8031: "rmii-ref": The RMII refence input clock. Used
> + to determine the XI input clock.
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 011dbda..492435f 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -26,6 +26,7 @@
> #include <linux/phy.h>
> #include <linux/micrel_phy.h>
> #include <linux/of.h>
> +#include <linux/clk.h>
>
> /* Operation Mode Strap Override */
> #define MII_KSZPHY_OMSO 0x16
> @@ -72,9 +73,12 @@ static int ksz_config_flags(struct phy_device *phydev)
> {
> int regval;
>
> - if (phydev->dev_flags & MICREL_PHY_50MHZ_CLK) {
> + if (phydev->dev_flags & (MICREL_PHY_50MHZ_CLK | MICREL_PHY_25MHZ_CLK)) {
> regval = phy_read(phydev, MII_KSZPHY_CTRL);
> - regval |= KSZ8051_RMII_50MHZ_CLK;
> + if (phydev->dev_flags & MICREL_PHY_50MHZ_CLK)
> + regval |= KSZ8051_RMII_50MHZ_CLK;
> + else
> + regval &= ~KSZ8051_RMII_50MHZ_CLK;
> return phy_write(phydev, MII_KSZPHY_CTRL, regval);
> }
> return 0;
> @@ -440,6 +444,27 @@ ksz9021_wr_mmd_phyreg(struct phy_device *phydev, int ptrad, int devnum,
> {
> }
>
> +static int ksz8021_probe(struct phy_device *phydev)
> +{
> + struct clk *clk;
> +
> + clk = devm_clk_get(&phydev->dev, "rmii-ref");
> + if (!IS_ERR(clk)) {
> + unsigned long rate = clk_get_rate(clk);
> +
> + if (rate > 24500000 && rate < 25500000) {
> + phydev->dev_flags |= MICREL_PHY_25MHZ_CLK;
> + } else if (rate > 49500000 && rate < 50500000) {
> + phydev->dev_flags |= MICREL_PHY_50MHZ_CLK;
> + } else {
> + dev_err(&phydev->dev, "Clock rate out of range: %ld\n", rate);
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +
> static struct phy_driver ksphy_driver[] = {
> {
> .phy_id = PHY_ID_KS8737,
> @@ -462,6 +487,7 @@ static struct phy_driver ksphy_driver[] = {
> .features = (PHY_BASIC_FEATURES | SUPPORTED_Pause |
> SUPPORTED_Asym_Pause),
> .flags = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
> + .probe = ksz8021_probe,
> .config_init = ksz8021_config_init,
> .config_aneg = genphy_config_aneg,
> .read_status = genphy_read_status,
> @@ -477,6 +503,7 @@ static struct phy_driver ksphy_driver[] = {
> .features = (PHY_BASIC_FEATURES | SUPPORTED_Pause |
> SUPPORTED_Asym_Pause),
> .flags = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
> + .probe = ksz8021_probe,
> .config_init = ksz8021_config_init,
> .config_aneg = genphy_config_aneg,
> .read_status = genphy_read_status,
> diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
> index 2e5b194..53d33de 100644
> --- a/include/linux/micrel_phy.h
> +++ b/include/linux/micrel_phy.h
> @@ -37,6 +37,7 @@
>
> /* struct phy_device dev_flags definitions */
> #define MICREL_PHY_50MHZ_CLK 0x00000001
> +#define MICREL_PHY_25MHZ_CLK 0x00000002
>
> #define MICREL_KSZ9021_EXTREG_CTRL 0xB
> #define MICREL_KSZ9021_EXTREG_DATA_WRITE 0xC
>
^ permalink raw reply
* RE: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports
From: David Laight @ 2014-10-10 16:30 UTC (permalink / raw)
To: 'Eric Dumazet'
Cc: 'alexander.h.duyck@redhat.com', David Miller,
alexander.duyck@gmail.com, netdev@vger.kernel.org
In-Reply-To: <1412955219.9362.13.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: 10 October 2014 16:34
> To: David Laight
> Cc: 'alexander.h.duyck@redhat.com'; David Miller; alexander.duyck@gmail.com; netdev@vger.kernel.org
> Subject: Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports
>
> On Fri, 2014-10-10 at 14:57 +0000, David Laight wrote:
>
> > I think there is code to copy the IP and TCP headers to aligned memory
> > before they are parsed.
>
> There is no such thing. You are here on netdev list, please read the
> code before doing such claims.
I did say 'I think'...
I must be thinking of some similar code somewhere else.
Possibly just the code that ensures the header isn't fragmented.
> > > The problem is the igb / ixgbe / fm10k hardware doesn't have a means of
> > > inserting padding from its side...
> >
> > Shoot the hardware engineers.
> >
> > You aren't going to get the performance you expect from a 10Ge card
> > unless the rx buffers are 'correctly' aligned.
>
> That is simply not true on current x86 cpus. They simply dont care at
> all.
I was referring to using them on sparc64, not x86.
I know that current intel x86 cpu have support for misaligned 'rep movsd',
but I thought there was still a small cost (maybe one clock) for
single word transfers.
So maybe they care 'just a little bit'.
> You cannot blame Intel for other arches.
True, but this does mean that you don't really want to use these adapters
on a system that can't to unaligned accesses.
David
^ permalink raw reply
* Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks
From: Neil Horman @ 2014-10-10 15:39 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, linux-sctp, netdev, Vlad Yasevich
In-Reply-To: <1412888133-833-3-git-send-email-dborkman@redhat.com>
On Thu, Oct 09, 2014 at 10:55:32PM +0200, Daniel Borkmann wrote:
> When receiving a e.g. semi-good formed connection scan in the
> form of ...
>
> -------------- INIT[ASCONF; ASCONF_ACK] ------------->
> <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
> -------------------- COOKIE-ECHO -------------------->
> <-------------------- COOKIE-ACK ---------------------
> ---------------- ASCONF_a; ASCONF_b ----------------->
>
> ... where ASCONF_a equals ASCONF_b chunk (at least both serials
> need to be equal), we panic an SCTP server!
>
> The problem is that good-formed ASCONF chunks that we reply with
> ASCONF_ACK chunks are cached per serial. Thus, when we receive a
> same ASCONF chunk twice (e.g. through a lost ASCONF_ACK), we do
> not need to process them again on the server side (that was the
> idea, also proposed in the RFC). Instead, we know it was cached
> and we just resend the cached chunk instead. So far, so good.
>
> Where things get nasty is in SCTP's side effect interpreter, that
> is, sctp_cmd_interpreter():
>
> While incoming ASCONF_a (chunk = event_arg) is being marked
> !end_of_packet and !singleton, and we have an association context,
> we do not flush the outqueue the first time after processing the
> ASCONF_ACK singleton chunk via SCTP_CMD_REPLY. Instead, we keep it
> queued up, although we set local_cork to 1. Commit 2e3216cd54b1
> changed the precedence, so that as long as we get bundled, incoming
> chunks we try possible bundling on outgoing queue as well. Before
> this commit, we would just flush the output queue.
>
> Now, while ASCONF_a's ASCONF_ACK sits in the corked outq, we
> continue to process the same ASCONF_b chunk from the packet. As
> we have cached the previous ASCONF_ACK, we find it, grab it and
> do another SCTP_CMD_REPLY command on it. So, effectively, we rip
> the chunk->list pointers and requeue the same ASCONF_ACK chunk
> another time. Since we process ASCONF_b, it's correctly marked
> with end_of_packet and we enforce an uncork, and thus flush, thus
> crashing the kernel.
>
> Fix it by testing if the ASCONF_ACK is currently pending and if
> that is the case, do not requeue it. When flushing the output
> queue we may relink the chunk for preparing an outgoing packet,
> but eventually unlink it when it's copied into the skb right
> before transmission.
>
> Joint work with Vlad Yasevich.
>
> Fixes: 2e3216cd54b1 ("sctp: Follow security requirement of responding with 1 packet")
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
> ---
> include/net/sctp/sctp.h | 5 +++++
> net/sctp/associola.c | 2 ++
> 2 files changed, 7 insertions(+)
>
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 9fbd856..856f01c 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -426,6 +426,11 @@ static inline void sctp_assoc_pending_pmtu(struct sock *sk, struct sctp_associat
> asoc->pmtu_pending = 0;
> }
>
> +static inline bool sctp_chunk_pending(const struct sctp_chunk *chunk)
> +{
> + return !list_empty(&chunk->list);
> +}
> +
> /* Walk through a list of TLV parameters. Don't trust the
> * individual parameter lengths and instead depend on
> * the chunk length to indicate when to stop. Make sure
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index a88b852..f791edd 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1668,6 +1668,8 @@ struct sctp_chunk *sctp_assoc_lookup_asconf_ack(
> * ack chunk whose serial number matches that of the request.
> */
> list_for_each_entry(ack, &asoc->asconf_ack_list, transmitted_list) {
> + if (sctp_chunk_pending(ack))
> + continue;
> if (ack->subh.addip_hdr->serial == serial) {
> sctp_chunk_hold(ack);
> return ack;
> --
> 1.7.11.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
received with duplicate serials?
Neil
^ permalink raw reply
* Re: [PATCH v2] flow-dissector: Fix alignment issue in __skb_flow_get_ports
From: Eric Dumazet @ 2014-10-10 15:36 UTC (permalink / raw)
To: alexander.duyck; +Cc: netdev, davem
In-Reply-To: <20141010145647.23006.54451.stgit@ahduyck-workstation.home>
On Fri, 2014-10-10 at 07:59 -0700, alexander.duyck@gmail.com wrote:
> From: Alexander Duyck <alexander.h.duyck@redhat.com>
>
> This patch addresses a kernel unaligned access bug seen on a sparc64 system
> with an igb adapter. Specifically the __skb_flow_get_ports was returning a
> be32 pointer which was then having the value directly returned.
>
> In order to keep the handling of the ports consistent with how we were
> handling the IPv4 and IPv6 addresses I have instead replaced the assignment
> with a memcpy to the flow key ports value. This way it should stay a
> memcpy on systems that cannot handle unaligned access, and will likely be
> converted to a 32b assignment on the systems that can support it.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
I believe you also need to take care of calls to ipv6_addr_hash()
The IPv4 part also needs something in iph_to_flow_copy_addrs(),
otherwise compiler might assume &iph->saddr is word aligned.
^ permalink raw reply
* Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports
From: Eric Dumazet @ 2014-10-10 15:33 UTC (permalink / raw)
To: David Laight
Cc: 'alexander.h.duyck@redhat.com', David Miller,
alexander.duyck@gmail.com, netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D174C858B@AcuExch.aculab.com>
On Fri, 2014-10-10 at 14:57 +0000, David Laight wrote:
> I think there is code to copy the IP and TCP headers to aligned memory
> before they are parsed.
There is no such thing. You are here on netdev list, please read the
code before doing such claims.
> ...
> >
> > The problem is the igb / ixgbe / fm10k hardware doesn't have a means of
> > inserting padding from its side...
>
> Shoot the hardware engineers.
>
> You aren't going to get the performance you expect from a 10Ge card
> unless the rx buffers are 'correctly' aligned.
That is simply not true on current x86 cpus. They simply dont care at
all.
You cannot blame Intel for other arches.
^ permalink raw reply
* Re: [ovs-dev] [PATCH/RFC repost 2/8] netlink: Allow suppression of warnings for duplicate attributes
From: Ben Pfaff @ 2014-10-10 15:31 UTC (permalink / raw)
To: Simon Horman; +Cc: dev, netdev
In-Reply-To: <20141009011831.GF9339@vergenet.net>
On Thu, Oct 09, 2014 at 10:18:32AM +0900, Simon Horman wrote:
> On Fri, Sep 26, 2014 at 04:55:42PM -0700, Ben Pfaff wrote:
> > On Thu, Sep 18, 2014 at 10:55:05AM +0900, Simon Horman wrote:
> > > Add a multiple field to struct nl_policy which if set suppresses
> > > warning of duplicate attributes in nl_parse_nested().
> > >
> > > As is the case without this patch only the last occurrence of an
> > > attribute is stored in attrs by nl_parse_nested(). As such
> > > if the multiple field of struct nl_policy is set then it
> > > is up to the caller to parse the message to extract all the attributes.
> > >
> > > This is in preparation for allowing multiple OVS_SELECT_GROUP_ATTR_BUCKET
> > > attributes in a nested OVS_ACTION_ATTR_SELECT_GROUP attribute.
> > >
> > > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> >
> > In the other case where we have duplicate attributes, it doesn't make
> > sense to process them with the policy functions, because we want to
> > see all of the instances of the duplicate attributes and policy
> > doesn't allow us to do that. I'm a little surprised that the new
> > attributes work differently. What's the idea?
>
> My idea was to use the policy to obtain the attributes that
> may not be duplicated. And then custom code to pick up all the
> instances of attributes that may be duplicated.
>
> I'm don't feel strongly about that approach and I'd be just has
> happy to drop this patch and rework things a little so that
> all the attributes are picked out by custom code. It sounds
> like that would match the approach taken elsewhere. Sorry for
> not noticing that earlier.
I see.
That's more like the approach used elsewhere, yes, but in those cases
there wasn't anything (if I recall correctly) that could be usefully
done with the policy parser, so it wasn't considered as an option. If
the group buckets are different then maybe a different treatment is
warranted.
^ permalink raw reply
* Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports
From: Eric Dumazet @ 2014-10-10 15:29 UTC (permalink / raw)
To: alexander.h.duyck
Cc: David Laight, David Miller, alexander.duyck@gmail.com,
netdev@vger.kernel.org
In-Reply-To: <5437F7CB.5010101@redhat.com>
On Fri, 2014-10-10 at 08:14 -0700, Alexander Duyck wrote:
> I think you might be coming to this a little late. The igb and ixgbe
> drivers had been working this way for a long time. We did a memcpy to
> move the headers from the page and into the skb->data at an aligned
> offset. In order to determine the length to memcpy we had a function
> that could walk through the DMA aligned data to get the header length.
> The function for that was replaced with the __skb_flow_dissect as it was
> considered a duplication of code with the flow_dissection functions.
> However that is obviously not the case now that we are hitting these
> alignment issues.
>
> The question I have in all this is do I push forward and make
> __skb_flow_dissect work with unaligned accesses, or do I back off and
> put something equivilent to igb/ixgbe_get_headlen functions in the
> kernel in order to deal with the unaligned accesses as they had no
> issues with them since they were only concerned with getting the header
> length and kept all accesses 16b aligned.
>
I see nothing wrong dealing with unaligned accesses, as these helpers
are nop on x86 or CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y arches.
^ permalink raw reply
* Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports
From: Alexander Duyck @ 2014-10-10 15:14 UTC (permalink / raw)
To: David Laight, David Miller, alexander.duyck@gmail.com
Cc: netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D174C858B@AcuExch.aculab.com>
On 10/10/2014 07:57 AM, David Laight wrote:
> From: Alexander Duyck
>> On 10/09/2014 09:47 PM, David Miller wrote:
>>> From: alexander.duyck@gmail.com
>>> Date: Thu, 09 Oct 2014 21:03:28 -0700
>>>
>>>> From: Alexander Duyck <alexander.h.duyck@redhat.com>
>>>>
>>>> This patch addresses a kernel unaligned access bug seen on a sparc64 system
>>>> with an igb adapter. Specifically the __skb_flow_get_ports was returning a
>>>> be32 pointer which was then having the value directly returned.
>>>>
>>>> In order to keep the handling of the ports consistent with how we were
>>>> handling the IPv4 and IPv6 addresses I have instead replaced the assignment
>>>> with a memcpy to the flow key ports value. This way it should stay a
>>>> memcpy on systems that cannot handle unaligned access, and will likely be
>>>> converted to a 32b assignment on the systems that can support it.
>>>>
>>>> Reported-by: David S. Miller <davem@davemloft.net>
>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>>> Guess what the compiler will output for the memcpy()....
>>>
>>> *(u32 *)dest = *(u32 *)src;
>>>
>>> Using memcpy() is never a valid way to avoid misaligned loads and
>>> stores.
>> If needed we could convert ports to a __be16 I suppose.
> You would have to cast it somewhere where the compiler cannot
> tell what the original type was.
> This usually means you have to call a non-static function, which
> might have to be in a different compilation unit.
>
> ...
The pointer itself gets assigned from skb->data + offset, since
skb->data is an unsigned char I would think that it should be safe to
cast it as a __be16 and have that stick. It is only if we cast it as a
__be32 that it should be an issue as we are casting a value that starts
off only byte aligned.
>> That is what I get for trying to come up with a fix at the end of the
>> day. Although it does leave me scratching my head why the IPv4 and IPv6
>> addresses were not causing unaligned accesses or were they triggering
>> them as well?
> I think there is code to copy the IP and TCP headers to aligned memory
> before they are parsed.
>
> ...
The code I am talking about is run before we actually get into the
header parsing. So for example if you take a look at
iph_to_flow_copy_addrs that should be getting called before we even get
to the code that is supposedly triggering this and it is doing 32b
aligned accesses for the source and destination IP addresses.
>> The problem is the igb / ixgbe / fm10k hardware doesn't have a means of
>> inserting padding from its side...
> Shoot the hardware engineers.
>
> You aren't going to get the performance you expect from a 10Ge card
> unless the rx buffers are 'correctly' aligned.
>
> David
>
I think you might be coming to this a little late. The igb and ixgbe
drivers had been working this way for a long time. We did a memcpy to
move the headers from the page and into the skb->data at an aligned
offset. In order to determine the length to memcpy we had a function
that could walk through the DMA aligned data to get the header length.
The function for that was replaced with the __skb_flow_dissect as it was
considered a duplication of code with the flow_dissection functions.
However that is obviously not the case now that we are hitting these
alignment issues.
The question I have in all this is do I push forward and make
__skb_flow_dissect work with unaligned accesses, or do I back off and
put something equivilent to igb/ixgbe_get_headlen functions in the
kernel in order to deal with the unaligned accesses as they had no
issues with them since they were only concerned with getting the header
length and kept all accesses 16b aligned.
Thanks,
Alex
^ permalink raw reply
* Re: [PATCH 1/1] Checkpatch: coding style errors in Nvidia ethernet driver
From: Joe Perches @ 2014-10-10 15:03 UTC (permalink / raw)
To: Akshay Sarode; +Cc: John Stultz, netdev, linux-kernel
In-Reply-To: <1412928102-1696-1-git-send-email-akshaysarode21@gmail.com>
On Fri, 2014-10-10 at 13:31 +0530, Akshay Sarode wrote:
> ERROR: "foo* bar" should be "foo *bar"
> ERROR: do not initialise statics to 0 or NULL
> CHECK: spinlock_t definition without comment
> Signed-off-by: Akshay Sarode <akshaysarode21@gmail.com>
[]
> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
[]
> @@ -911,12 +913,18 @@ enum {
[]
> /*
> * Debug output control for tx_timeout
> */
> -static bool debug_tx_timeout = false;
> +enum {
> + NV_DEBUG_TX_TIMEOUT_DISABLED,
> + NV_DEBUG_TX_TIMEOUT_ENABLED
> +};
> +
> +static bool debug_tx_timeout = NV_DEBUG_TX_TIMEOUT_DISABLED;
Adding this enum is not useful.
^ permalink raw reply
* [PATCH v2] flow-dissector: Fix alignment issue in __skb_flow_get_ports
From: alexander.duyck @ 2014-10-10 14:59 UTC (permalink / raw)
To: netdev, davem
In-Reply-To: <20141009.201248.1210454965155680255.davem@davemloft.net>
From: Alexander Duyck <alexander.h.duyck@redhat.com>
This patch addresses a kernel unaligned access bug seen on a sparc64 system
with an igb adapter. Specifically the __skb_flow_get_ports was returning a
be32 pointer which was then having the value directly returned.
In order to keep the handling of the ports consistent with how we were
handling the IPv4 and IPv6 addresses I have instead replaced the assignment
with a memcpy to the flow key ports value. This way it should stay a
memcpy on systems that cannot handle unaligned access, and will likely be
converted to a 32b assignment on the systems that can support it.
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
v2: Changed ports to a __be16 * to prevent optimization for __be32 *
drivers/net/bonding/bond_main.c | 2 +-
include/net/flow_keys.h | 10 ++++++----
net/core/flow_dissector.c | 22 ++++++++++++++--------
3 files changed, 21 insertions(+), 13 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index c9ac06c..326eb3d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2993,7 +2993,7 @@ static bool bond_flow_dissect(struct bonding *bond, struct sk_buff *skb,
return false;
}
if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 && proto >= 0)
- fk->ports = skb_flow_get_ports(skb, noff, proto);
+ skb_flow_get_ports(fk, skb, noff, proto);
return true;
}
diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
index 7ee2df0..66235f4 100644
--- a/include/net/flow_keys.h
+++ b/include/net/flow_keys.h
@@ -33,11 +33,13 @@ static inline bool skb_flow_dissect(const struct sk_buff *skb, struct flow_keys
{
return __skb_flow_dissect(skb, flow, NULL, 0, 0, 0);
}
-__be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
- void *data, int hlen_proto);
-static inline __be32 skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto)
+void __skb_flow_get_ports(struct flow_keys *flow, const struct sk_buff *skb,
+ int thoff, u8 ip_proto, void *data, int hlen_proto);
+static inline void skb_flow_get_ports(struct flow_keys *flow,
+ const struct sk_buff *skb, int thoff,
+ u8 ip_proto)
{
- return __skb_flow_get_ports(skb, thoff, ip_proto, NULL, 0);
+ __skb_flow_get_ports(flow, skb, thoff, ip_proto, NULL, 0);
}
u32 flow_hash_from_keys(struct flow_keys *keys);
unsigned int flow_get_hlen(const unsigned char *data, unsigned int max_len,
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 8560dea..e789a39 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -28,6 +28,7 @@ static void iph_to_flow_copy_addrs(struct flow_keys *flow, const struct iphdr *i
/**
* __skb_flow_get_ports - extract the upper layer ports and return them
+ * @flow: Flow key to place port informatin into
* @skb: sk_buff to extract the ports from
* @thoff: transport header offset
* @ip_proto: protocol for which to get port offset
@@ -37,8 +38,8 @@ static void iph_to_flow_copy_addrs(struct flow_keys *flow, const struct iphdr *i
* The function will try to retrieve the ports at offset thoff + poff where poff
* is the protocol port offset returned from proto_ports_offset
*/
-__be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
- void *data, int hlen)
+void __skb_flow_get_ports(struct flow_keys *flow, const struct sk_buff *skb,
+ int thoff, u8 ip_proto, void *data, int hlen)
{
int poff = proto_ports_offset(ip_proto);
@@ -48,15 +49,20 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
}
if (poff >= 0) {
- __be32 *ports, _ports;
+ __be16 *ports;
ports = __skb_header_pointer(skb, thoff + poff,
- sizeof(_ports), data, hlen, &_ports);
- if (ports)
- return *ports;
+ sizeof(flow->ports), data, hlen,
+ &flow->ports);
+ if (ports) {
+ if (flow->port16 != ports)
+ memcpy(&flow->ports, ports,
+ sizeof(flow->ports));
+ return;
+ }
}
- return 0;
+ memset(&flow->ports, 0, sizeof(flow->ports));
}
EXPORT_SYMBOL(__skb_flow_get_ports);
@@ -231,7 +237,7 @@ ipv6:
flow->n_proto = proto;
flow->ip_proto = ip_proto;
- flow->ports = __skb_flow_get_ports(skb, nhoff, ip_proto, data, hlen);
+ __skb_flow_get_ports(flow, skb, nhoff, ip_proto, data, hlen);
flow->thoff = (u16) nhoff;
return true;
^ permalink raw reply related
* RE: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports
From: David Laight @ 2014-10-10 14:57 UTC (permalink / raw)
To: 'alexander.h.duyck@redhat.com', David Miller,
alexander.duyck@gmail.com
Cc: netdev@vger.kernel.org
In-Reply-To: <5437F072.5060502@redhat.com>
From: Alexander Duyck
> On 10/09/2014 09:47 PM, David Miller wrote:
> > From: alexander.duyck@gmail.com
> > Date: Thu, 09 Oct 2014 21:03:28 -0700
> >
> >> From: Alexander Duyck <alexander.h.duyck@redhat.com>
> >>
> >> This patch addresses a kernel unaligned access bug seen on a sparc64 system
> >> with an igb adapter. Specifically the __skb_flow_get_ports was returning a
> >> be32 pointer which was then having the value directly returned.
> >>
> >> In order to keep the handling of the ports consistent with how we were
> >> handling the IPv4 and IPv6 addresses I have instead replaced the assignment
> >> with a memcpy to the flow key ports value. This way it should stay a
> >> memcpy on systems that cannot handle unaligned access, and will likely be
> >> converted to a 32b assignment on the systems that can support it.
> >>
> >> Reported-by: David S. Miller <davem@davemloft.net>
> >> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> > Guess what the compiler will output for the memcpy()....
> >
> > *(u32 *)dest = *(u32 *)src;
> >
> > Using memcpy() is never a valid way to avoid misaligned loads and
> > stores.
>
> If needed we could convert ports to a __be16 I suppose.
You would have to cast it somewhere where the compiler cannot
tell what the original type was.
This usually means you have to call a non-static function, which
might have to be in a different compilation unit.
...
> That is what I get for trying to come up with a fix at the end of the
> day. Although it does leave me scratching my head why the IPv4 and IPv6
> addresses were not causing unaligned accesses or were they triggering
> them as well?
I think there is code to copy the IP and TCP headers to aligned memory
before they are parsed.
...
>
> The problem is the igb / ixgbe / fm10k hardware doesn't have a means of
> inserting padding from its side...
Shoot the hardware engineers.
You aren't going to get the performance you expect from a 10Ge card
unless the rx buffers are 'correctly' aligned.
David
^ permalink raw reply
* Re: [PATCH net 1/3] net: sctp: fix skb_over_panic when receiving malformed ASCONF chunks
From: Neil Horman @ 2014-10-10 14:48 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, linux-sctp, netdev, Vlad Yasevich
In-Reply-To: <1412888133-833-2-git-send-email-dborkman@redhat.com>
On Thu, Oct 09, 2014 at 10:55:31PM +0200, Daniel Borkmann wrote:
> Commit 6f4c618ddb0 ("SCTP : Add paramters validity check for
> ASCONF chunk") added basic verification of ASCONF chunks, however,
> it is still possible to remotely crash a server by sending a
> special crafted ASCONF chunk, even up to pre 2.6.12 kernels:
>
> skb_over_panic: text:ffffffffa01ea1c3 len:31056 put:30768
> head:ffff88011bd81800 data:ffff88011bd81800 tail:0x7950
> end:0x440 dev:<NULL>
> ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:129!
> [...]
> Call Trace:
> <IRQ>
> [<ffffffff8144fb1c>] skb_put+0x5c/0x70
> [<ffffffffa01ea1c3>] sctp_addto_chunk+0x63/0xd0 [sctp]
> [<ffffffffa01eadaf>] sctp_process_asconf+0x1af/0x540 [sctp]
> [<ffffffff8152d025>] ? _read_unlock_bh+0x15/0x20
> [<ffffffffa01e0038>] sctp_sf_do_asconf+0x168/0x240 [sctp]
> [<ffffffffa01e3751>] sctp_do_sm+0x71/0x1210 [sctp]
> [<ffffffff8147645d>] ? fib_rules_lookup+0xad/0xf0
> [<ffffffffa01e6b22>] ? sctp_cmp_addr_exact+0x32/0x40 [sctp]
> [<ffffffffa01e8393>] sctp_assoc_bh_rcv+0xd3/0x180 [sctp]
> [<ffffffffa01ee986>] sctp_inq_push+0x56/0x80 [sctp]
> [<ffffffffa01fcc42>] sctp_rcv+0x982/0xa10 [sctp]
> [<ffffffffa01d5123>] ? ipt_local_in_hook+0x23/0x28 [iptable_filter]
> [<ffffffff8148bdc9>] ? nf_iterate+0x69/0xb0
> [<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
> [<ffffffff8148bf86>] ? nf_hook_slow+0x76/0x120
> [<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
> [<ffffffff81496ded>] ip_local_deliver_finish+0xdd/0x2d0
> [<ffffffff81497078>] ip_local_deliver+0x98/0xa0
> [<ffffffff8149653d>] ip_rcv_finish+0x12d/0x440
> [<ffffffff81496ac5>] ip_rcv+0x275/0x350
> [<ffffffff8145c88b>] __netif_receive_skb+0x4ab/0x750
> [<ffffffff81460588>] netif_receive_skb+0x58/0x60
>
> This can be triggered e.g., through a simple scripted nmap
> connection scan injecting the chunk after the handshake, for
> example, ...
>
> -------------- INIT[ASCONF; ASCONF_ACK] ------------->
> <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
> -------------------- COOKIE-ECHO -------------------->
> <-------------------- COOKIE-ACK ---------------------
> ------------------ ASCONF; UNKNOWN ------------------>
>
> ... where ASCONF chunk of length 280 contains 2 parameters ...
>
> 1) Add IP address parameter (param length: 16)
> 2) Add/del IP address parameter (param length: 255)
>
> ... followed by an UNKNOWN chunk of e.g. 4 bytes. Here, the
> Address Parameter in the ASCONF chunk is even missing, too.
> This is just an example and similarly-crafted ASCONF chunks
> could be used just as well.
>
> The ASCONF chunk passes through sctp_verify_asconf() as all
> parameters passed sanity checks, and after walking, we ended
> up successfully at the chunk end boundary, and thus may invoke
> sctp_process_asconf(). Parameter walking is done with
> WORD_ROUND() to take padding into account.
>
> In sctp_process_asconf()'s TLV processing, we may fail in
> sctp_process_asconf_param() e.g., due to removal of the IP
> address that is also the source address of the packet containing
> the ASCONF chunk, and thus we need to add all TLVs after the
> failure to our ASCONF response to remote via helper function
> sctp_add_asconf_response(), which basically invokes a
> sctp_addto_chunk() adding the error parameters to the given
> skb.
>
> When walking to the next parameter this time, we proceed
> with ...
>
> length = ntohs(asconf_param->param_hdr.length);
> asconf_param = (void *)asconf_param + length;
>
> ... instead of the WORD_ROUND()'ed length, thus resulting here
> in an off-by-one that leads to reading the follow-up garbage
> parameter length of 12336, and thus throwing an skb_over_panic
> for the reply when trying to sctp_addto_chunk() next time,
> which implicitly calls the skb_put() with that length.
>
> Fix it by using sctp_walk_params() [ which is also used in
> INIT parameter processing ] macro in the verification *and*
> in ASCONF processing: it will make sure we don't spill over,
> that we walk parameters WORD_ROUND()'ed. Moreover, we're being
> more defensive and guard against unknown parameter types and
> missized addresses.
>
> Joint work with Vlad Yasevich.
>
> Fixes: b896b82be4ae ("[SCTP] ADDIP: Support for processing incoming ASCONF_ACK chunks.")
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports
From: Alexander Duyck @ 2014-10-10 14:42 UTC (permalink / raw)
To: David Miller, alexander.duyck; +Cc: netdev
In-Reply-To: <20141010.004708.1975127853551765914.davem@davemloft.net>
On 10/09/2014 09:47 PM, David Miller wrote:
> From: alexander.duyck@gmail.com
> Date: Thu, 09 Oct 2014 21:03:28 -0700
>
>> From: Alexander Duyck <alexander.h.duyck@redhat.com>
>>
>> This patch addresses a kernel unaligned access bug seen on a sparc64 system
>> with an igb adapter. Specifically the __skb_flow_get_ports was returning a
>> be32 pointer which was then having the value directly returned.
>>
>> In order to keep the handling of the ports consistent with how we were
>> handling the IPv4 and IPv6 addresses I have instead replaced the assignment
>> with a memcpy to the flow key ports value. This way it should stay a
>> memcpy on systems that cannot handle unaligned access, and will likely be
>> converted to a 32b assignment on the systems that can support it.
>>
>> Reported-by: David S. Miller <davem@davemloft.net>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> Guess what the compiler will output for the memcpy()....
>
> *(u32 *)dest = *(u32 *)src;
>
> Using memcpy() is never a valid way to avoid misaligned loads and
> stores.
If needed we could convert ports to a __be16 I suppose.
> If the types have a given alignment, the compiler can legitimately
> expand the memcpy() inline with suitably sized loads and stores.
That is what I get for trying to come up with a fix at the end of the
day. Although it does leave me scratching my head why the IPv4 and IPv6
addresses were not causing unaligned accesses or were they triggering
them as well?
> Please see my other reply in the original thread, we have to use
> the hardware when we can to manage this situation, by configuring
> it to output two garbage bytes before the packet contents when
> DMA'ing into power-of-2 aligned blocks of memory.
>
> Thanks.
The problem is the igb / ixgbe / fm10k hardware doesn't have a means of
inserting padding from its side. The whole point of the xxx_get_headlen
functions was to determine the size needed in order to generate the
correct memcpy so that we could generate an aligned header. It sounds
like we can't do that with this function now because there are multiple
instances where the data will be accessed unaligned if it isn't aligned
in the first place.
The way I see it now there are two possible solutions. The first one
being to update the flow hash functions to work with unaligned accesses,
and the second being to split the get_headlen code flow off and for now
use my original code which already had all the alignment issues
resolved. I'm open to suggestions either way, but for now I will try to
just address the items you have pointed out here with the current patch
and submit a v2.
Thanks,
Alex
^ permalink raw reply
* Re: hso. hso_serial_close oops.
From: Peter Hurley @ 2014-10-10 14:22 UTC (permalink / raw)
To: christian.melki, Jan Dumon; +Cc: linux-usb, netdev@vger.kernel.org
In-Reply-To: <5437E6E7.7070107@t2data.com>
[ +Jan Dumon, netdev ]
Forwarding oops report to maintainer.
On 10/10/2014 10:02 AM, Christian Melki wrote:
> I'm using a ppc 8347 with a normal 3.16.1 kernel.
> The software opens the driver tty in question and then sets it as stdin, stdout for chat-program and pppd.
> When I yank the modem while running, the software detects this and tries to close the open socket with a kernel crash as a result.
>
> Unable to handle kernel paging request for unknown fault
> Faulting instruction address: 0xc03a4420
> Oops: Kernel access of bad area, sig: 11 [#1]
> PREEMPT ASP8347E
> Modules linked in:
> CPU: 0 PID: 1536 Comm: pppd Not tainted 3.16.1 #1
> task: c31272e0 ti: c39e8000 task.ti: c39e8000
> NIP: c03a4420 LR: c020752c CTR: c02074e0
> REGS: c39e9d40 TRAP: 0600 Not tainted (3.16.1)
> MSR: 00009032 <EE,ME,IR,DR,RI> CR: 24004224 XER: 20000000
> DAR: 0000004d DSISR: 00000000
> GPR00: 00000000 c39e9df0 c31272e0 0000004d c3235460 00000000 c39c1934 00000000
> GPR08: 00000000 00000000 c39c1964 c39c1800 24004228 10047610 10040000 1003f6ec
> GPR16: 1003f6b4 1003f618 1003f6b0 1003f6bc 1003f700 1003f7b4 c39e9edc 1003f6c8
> GPR24: 1003f6dc c03bd1a8 00000004 c03bd2b4 00000000 c3235460 00000000 c38cca00
> NIP [c03a4420] mutex_lock+0x0/0x1c
> LR [c020752c] hso_serial_close+0x4c/0x11c
> Call Trace:
> [c39e9df0] [c3235460] 0xc3235460 (unreliable)
> [c39e9e00] [c0188944] tty_release+0x134/0x560
> [c39e9e90] [c00a1968] __fput+0x94/0x214
> [c39e9eb0] [c0032854] task_work_run+0xcc/0xf4
> [c39e9ed0] [c0019108] do_exit+0x208/0x874
> [c39e9f20] [c00198c0] do_group_exit+0x44/0xd8
> [c39e9f30] [c0019968] __wake_up_parent+0x0/0x34
> [c39e9f40] [c000e60c] ret_from_syscall+0x0/0x38
> --- Exception: c01 at 0xfebd4cc
> LR = 0xff79a98
> Instruction dump:
> 409e0014 801f003c 70090004 41820008 4bffe6ad 80010034 bb410018 38210030
> 7c0803a6 4e800020 4bffe695 4bffffc4 <7c001828> 3000ffff 7c00192d 40a2fff4
> ---[ end trace bfebaf22f6f5795a ]---
>
> Fixing recursive fault but reboot is needed!
>
> I have simulated the same error with a simple userland program without using pppd.
>
>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <unistd.h>
> #include <stdlib.h>
>
> int
> main(int argc, char *argv[]) {
> int fd;
> fd = open(argv[1], O_RDWR);
> sleep(atoi(argv[2]));
> close(fd);
>
> return 0;
> }
>
> If I yank the modem while the program is sleeping, I get exactly the same kernel error as with pppd.
> I have looked at the hso.c (hso_serial_close) driver but can't figure it out. The structs might not be intact at that time, but those are tty structs.. Im not sure what is going on. I tried to check the integrity of the structs but still get a crash. The tty layer is a mystery to me.
>
> regards,
> Christian
^ permalink raw reply
* [PATCH] ipv6: notify userspace when we added or changed an ipv6 token
From: Lubomir Rintel @ 2014-10-10 14:08 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, David S. Miller, Lubomir Rintel,
Hannes Frederic Sowa, Daniel Borkmann
NetworkManager might want to know that it changed when the router advertisement
arrives.
Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Daniel Borkmann <dborkman@redhat.com>
---
net/ipv6/addrconf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 3e118df..3d11390 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4528,6 +4528,7 @@ static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
}
write_unlock_bh(&idev->lock);
+ netdev_state_change(dev);
addrconf_verify_rtnl();
return 0;
}
--
1.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox