* Re: [PATCH v3 13/19] media: dvb_core: slight optimization of addr compare
From: Mauro Carvalho Chehab @ 2014-01-07 13:54 UTC (permalink / raw)
To: Ding Tianhong
Cc: Sergei Shtylyov, linux-media, linux-kernel@vger.kernel.org,
Netdev
In-Reply-To: <52BC0E56.80003@huawei.com>
Em Thu, 26 Dec 2013 19:09:10 +0800
Ding Tianhong <dingtianhong@huawei.com> escreveu:
> On 2013/12/25 18:57, Sergei Shtylyov wrote:
> > Hello.
> >
> > On 25-12-2013 7:29, Ding Tianhong wrote:
> >
> >> Use possibly more efficient ether_addr_equal
> >> instead of memcmp.
> >
> >> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
> >> Cc: linux-media@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> >> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> >> ---
> >> drivers/media/dvb-core/dvb_net.c | 8 ++++----
> >> 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> >> diff --git a/drivers/media/dvb-core/dvb_net.c b/drivers/media/dvb-core/dvb_net.c
> >> index f91c80c..3dfc33b 100644
> >> --- a/drivers/media/dvb-core/dvb_net.c
> >> +++ b/drivers/media/dvb-core/dvb_net.c
> >> @@ -179,7 +179,7 @@ static __be16 dvb_net_eth_type_trans(struct sk_buff *skb,
> >> eth = eth_hdr(skb);
> >>
> >> if (*eth->h_dest & 1) {
> >> - if(memcmp(eth->h_dest,dev->broadcast, ETH_ALEN)==0)
> >> + if(ether_addr_equal(eth->h_dest,dev->broadcast))
> >
> > There should be space after comma.
> >
> >> @@ -674,11 +674,11 @@ static void dvb_net_ule( struct net_device *dev, const u8 *buf, size_t buf_len )
> >> if (priv->rx_mode != RX_MODE_PROMISC) {
> >> if (priv->ule_skb->data[0] & 0x01) {
> >> /* multicast or broadcast */
> >> - if (memcmp(priv->ule_skb->data, bc_addr, ETH_ALEN)) {
> >> + if (!ether_addr_equal(priv->ule_skb->data, bc_addr)) {
> >> /* multicast */
> >> if (priv->rx_mode == RX_MODE_MULTI) {
> >> int i;
> >> - for(i = 0; i < priv->multi_num && memcmp(priv->ule_skb->data, priv->multi_macs[i], ETH_ALEN); i++)
> >> + for(i = 0; i < priv->multi_num && !ether_addr_equal(priv->ule_skb->data, priv->multi_macs[i]); i++)
> >
> > Shouldn't this line be broken?
> >
>
> ok, thanks.
Also, since you're touching on those lines, could you please add an space
after 'if' (on the first hunk) and after 'for' (on the second one)?
>
> Regards
> >> ;
> >> if (i == priv->multi_num)
> >> drop = 1;
> >
> > WBR, Sergei
> >
> >
> >
> >
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks,
Mauro
^ permalink raw reply
* Re: single process receives own frames due to PACKET_MMAP
From: Daniel Borkmann @ 2014-01-07 13:47 UTC (permalink / raw)
To: Norbert van Bolhuis; +Cc: Jesper Dangaard Brouer, netdev, David Miller, uaca
In-Reply-To: <52CBFE13.8@aimvalley.nl>
On 01/07/2014 02:16 PM, Norbert van Bolhuis wrote:
> On 01/07/14 11:06, Jesper Dangaard Brouer wrote:
>> On Tue, 07 Jan 2014 10:32:01 +0100
>> Daniel Borkmann<dborkman@redhat.com> wrote:
>>> On 01/06/2014 11:58 PM, Norbert van Bolhuis wrote:
>>>>
>>>> Our application uses raw AF_PACKET socket to send and receive
>>>> on one particular ethernet interface.
>>>>
>>>> Recently we started using PACKET_MMAP (TPACKET_V2). This makes
>>>> the Appl use a TX socket and a RX socket.
>>>> Both sockets are bound to the same (eth) interface. I noticed
>>>> the RX socket receives all frames that are sent via the
>>>> TX socket (same process, different thread). This I do not want.
>>>>
>>>> I know it is supposed to happen for different processes
>>>> (otherwise wireshark won't work), but I did not expect it to
>>>> happen for one single process (with different threads).
>>>>
>>>> I can filter them out in user-space (PACKET_OUTGOING)
>>>> or via kernel packet filter (SO_ATTACH_FILTER), but performance is
>>>> critical.
>>>>
>>>> I wonder whether this (PACKET_MMAP) behaviour is OK.
>>>
>>> For your use-case, we recently introduced in d346a3fae3ff1
>>> ("packet: introduce PACKET_QDISC_BYPASS socket option") a
>>> bypass of dev_queue_xmit() (that internally invokes
>>> dev_queue_xmit_nit()).
>>>
>>>> It did not happen before (with a non-PACKET_MMAP AF_PACKET socket
>>>> which was used by both threads of the same Appl process). So
>>>> why is it happening now ?
>>>
>>> Can you elaborate a bit on which kernel versions that behaviour
>>> changed?
>
>
> Sorry, I wasn't very clear. This does not regard kernel (versions).
> With "before" I mean our previous version of our application which
> used an AF_PACKET socket, but not the PACKET_MMAP option.
Hmm, taking a look into 2.6.35.14, it doesn't seem to be so much of
a difference.
W/o seeing how you've invoked mmap vs non-mmap API, it's hard to say.
Could you try with latest upstream resp. net-next kernel?
>>>> I'd say it makes no sense to make the same process receive its
>>>> own transmitted frames on that same interface (unless its lo).
>>
>> Have you setup:
>> ring->s_ll.sll_protocol = 0
>>
>> This is what I did in trafgen to avoid this problem.
>>
>> See line 55 in netsniff-ng/ring.c:
>> https://github.com/borkmann/netsniff-ng/blob/c3602a995b21e8133c7f4fd1fb1e7e21b6a844f1/ring.c#L55
>>
>> Commit:
>> https://github.com/borkmann/netsniff-ng/commit/c3602a995b21e8133c7f4fd1fb1e7e21b6a844f1
>>
>
>
> No I did not do that, I was checking my code against netsniff-ng-0.5.8-rc4.
>
> But I just tried it, I believe I do the same as netsniff-ng-0.5.8-rc5, but it doesn't
> work for me. Maybe because I have an old FC14 system (kernel 2.6.35.14-106.fc14.x86_64).
>
> So I tried to see whether netsniff-ng-0.5.8-rc5/trafgen still makes the
> kernel call packet_rcv() on my FC14 system. So I build and run it, but I'm not sure
> how to (easily) check that. In anyway, Wireshark does capture the trafgen generated
> frames, does that say anything ?
Yep, kernel is definitely too old for that.
> In the future, I can at least use PACKET_QDISC_BYPASS as a "workaround".
>
> Thanks a lot for your answers.
>
> ---
> Norbert
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH -next] qlcnic: use vzalloc() instead of vmalloc()/memset(0)
From: Wei Yongjun @ 2014-01-07 13:48 UTC (permalink / raw)
To: himanshu.madhani, rajesh.borundia, shahed.shaikh,
jitendra.kalsaria, sony.chacko, sucheta.chakraborty
Cc: yongjun_wei, linux-driver, netdev
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Use vzalloc() instead of vmalloc() and memset(0).
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
drivers/net/ethernet/qlogic/qlcnic/qlcnic_sysfs.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sysfs.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sysfs.c
index b529667..16912e2 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sysfs.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sysfs.c
@@ -935,11 +935,10 @@ static ssize_t qlcnic_sysfs_read_pci_config(struct file *file,
return QL_STATUS_INVALID_PARAM;
pci_info_sz = pci_func_count * sizeof(*pci_info);
- pci_info = vmalloc(pci_info_sz);
+ pci_info = vzalloc(pci_info_sz);
if (!pci_info)
return -ENOMEM;
- memset(pci_info, 0, pci_info_sz);
memset(buf, 0, pci_cfg_sz);
pci_cfg = (struct qlcnic_pci_func_cfg *)buf;
^ permalink raw reply related
* [PATCH 2/2] utils: add test for nfq_get_uid and nfq_get_gid
From: valentina.giusti @ 2014-01-07 13:30 UTC (permalink / raw)
To: pablo, netfilter-devel; +Cc: netdev, fw, daniel.wagner
In-Reply-To: <1389101420-16557-1-git-send-email-valentina.giusti@bmw-carit.de>
From: Valentina Giusti <Valentina.Giusti@bmw-carit.de>
Signed-off-by: Valentina Giusti <Valentina.Giusti@bmw-carit.de>
---
utils/nfqnl_test.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/utils/nfqnl_test.c b/utils/nfqnl_test.c
index a554f2d..c6b97c6 100644
--- a/utils/nfqnl_test.c
+++ b/utils/nfqnl_test.c
@@ -15,7 +15,7 @@ static u_int32_t print_pkt (struct nfq_data *tb)
int id = 0;
struct nfqnl_msg_packet_hdr *ph;
struct nfqnl_msg_packet_hw *hwph;
- u_int32_t mark,ifi;
+ u_int32_t mark, ifi, uid, gid;
int ret;
unsigned char *data;
@@ -55,6 +55,12 @@ static u_int32_t print_pkt (struct nfq_data *tb)
if (ifi)
printf("physoutdev=%u ", ifi);
+ if (nfq_get_uid(tb, &uid))
+ printf("uid=%u ", uid);
+
+ if (nfq_get_gid(tb, &gid))
+ printf("gid=%u ", gid);
+
ret = nfq_get_payload(tb, &data);
if (ret >= 0)
printf("payload_len=%d ", ret);
@@ -114,6 +120,10 @@ int main(int argc, char **argv)
exit(1);
}
+ printf("setting flags to request UID and GID\n");
+ if (nfq_set_queue_flags(qh, NFQA_CFG_F_UID_GID, NFQA_CFG_F_UID_GID))
+ fprintf(stderr, "can't request UID_GID flag\n");
+
fd = nfq_fd(h);
for (;;) {
--
1.8.5.1
^ permalink raw reply related
* [PATCH 0/2] libnetfilter_queue: add support for UID/GID socket info
From: valentina.giusti @ 2014-01-07 13:30 UTC (permalink / raw)
To: pablo, netfilter-devel; +Cc: netdev, fw, daniel.wagner
In-Reply-To: <20131221113804.GA17964@localhost>
From: Valentina Giusti <Valentina.Giusti@bmw-carit.de>
This patchset contains the following changes:
* re-implemented nfq_get_uid and nfq_get_gid as suggested by Pablo Neira Ayuso
* corrected enum nfqnl_attr_type in linux_nfnetlink_queue.h
* added a test for the aforementioned new APIs in utils/nfqnl_test.c
Best Regards,
Valentina Giusti (2):
src: add support for UID/GID socket info
utils: add test for nfq_get_uid and nfq_get_gid
include/libnetfilter_queue/libnetfilter_queue.h | 4 ++
include/libnetfilter_queue/linux_nfnetlink_queue.h | 6 ++-
include/linux/netfilter/nfnetlink_queue.h | 6 ++-
src/libnetfilter_queue.c | 60 ++++++++++++++++++++++
src/nlmsg.c | 2 +
utils/nfqnl_test.c | 14 ++++-
6 files changed, 89 insertions(+), 3 deletions(-)
--
1.8.5.1
^ permalink raw reply
* [PATCH 1/2] src: add support for UID/GID socket info
From: valentina.giusti @ 2014-01-07 13:30 UTC (permalink / raw)
To: pablo, netfilter-devel; +Cc: netdev, fw, daniel.wagner
In-Reply-To: <1389101420-16557-1-git-send-email-valentina.giusti@bmw-carit.de>
From: Valentina Giusti <Valentina.Giusti@bmw-carit.de>
With this patch libnetfilter_queue is able to parse UID/GID
socket information.
Signed-off-by: Valentina Giusti <Valentina.Giusti@bmw-carit.de>
---
include/libnetfilter_queue/libnetfilter_queue.h | 4 ++
include/libnetfilter_queue/linux_nfnetlink_queue.h | 6 ++-
include/linux/netfilter/nfnetlink_queue.h | 6 ++-
src/libnetfilter_queue.c | 46 ++++++++++++++++++++++
src/nlmsg.c | 2 +
5 files changed, 62 insertions(+), 2 deletions(-)
diff --git a/include/libnetfilter_queue/libnetfilter_queue.h b/include/libnetfilter_queue/libnetfilter_queue.h
index b9f16e2..122cd10 100644
--- a/include/libnetfilter_queue/libnetfilter_queue.h
+++ b/include/libnetfilter_queue/libnetfilter_queue.h
@@ -103,6 +103,8 @@ extern u_int32_t nfq_get_indev(struct nfq_data *nfad);
extern u_int32_t nfq_get_physindev(struct nfq_data *nfad);
extern u_int32_t nfq_get_outdev(struct nfq_data *nfad);
extern u_int32_t nfq_get_physoutdev(struct nfq_data *nfad);
+extern int nfq_get_uid(struct nfq_data *nfad, u_int32_t *uid);
+extern int nfq_get_gid(struct nfq_data *nfad, u_int32_t *gid);
extern int nfq_get_indev_name(struct nlif_handle *nlif_handle,
struct nfq_data *nfad, char *name);
@@ -125,6 +127,8 @@ enum {
NFQ_XML_PHYSDEV = (1 << 3),
NFQ_XML_PAYLOAD = (1 << 4),
NFQ_XML_TIME = (1 << 5),
+ NFQ_XML_UID = (1 << 6),
+ NFQ_XML_GID = (1 << 7),
NFQ_XML_ALL = ~0U,
};
diff --git a/include/libnetfilter_queue/linux_nfnetlink_queue.h b/include/libnetfilter_queue/linux_nfnetlink_queue.h
index 81a485b..f732201 100644
--- a/include/libnetfilter_queue/linux_nfnetlink_queue.h
+++ b/include/libnetfilter_queue/linux_nfnetlink_queue.h
@@ -49,6 +49,9 @@ enum nfqnl_attr_type {
NFQA_CT_INFO, /* enum ip_conntrack_info */
NFQA_CAP_LEN, /* __u32 length of captured packet */
NFQA_SKB_INFO, /* __u32 skb meta information */
+ NFQA_EXP, /* nf_conntrack_netlink.h */
+ NFQA_UID, /* __u32 sk uid */
+ NFQA_GID, /* __u32 sk gid */
__NFQA_MAX
};
@@ -101,7 +104,8 @@ enum nfqnl_attr_config {
#define NFQA_CFG_F_FAIL_OPEN (1 << 0)
#define NFQA_CFG_F_CONNTRACK (1 << 1)
#define NFQA_CFG_F_GSO (1 << 2)
-#define NFQA_CFG_F_MAX (1 << 3)
+#define NFQA_CFG_F_UID_GID (1 << 3)
+#define NFQA_CFG_F_MAX (1 << 4)
/* flags for NFQA_SKB_INFO */
/* packet appears to have wrong checksums, but they are ok */
diff --git a/include/linux/netfilter/nfnetlink_queue.h b/include/linux/netfilter/nfnetlink_queue.h
index a2308ae..22f5d45 100644
--- a/include/linux/netfilter/nfnetlink_queue.h
+++ b/include/linux/netfilter/nfnetlink_queue.h
@@ -46,6 +46,9 @@ enum nfqnl_attr_type {
NFQA_CT_INFO, /* enum ip_conntrack_info */
NFQA_CAP_LEN, /* __u32 length of captured packet */
NFQA_SKB_INFO, /* __u32 skb meta information */
+ NFQA_EXP, /* nf_conntrack_netlink.h */
+ NFQA_UID, /* __u32 sk uid */
+ NFQA_GID, /* __u32 sk gid */
__NFQA_MAX
};
@@ -98,7 +101,8 @@ enum nfqnl_attr_config {
#define NFQA_CFG_F_FAIL_OPEN (1 << 0)
#define NFQA_CFG_F_CONNTRACK (1 << 1)
#define NFQA_CFG_F_GSO (1 << 2)
-#define NFQA_CFG_F_MAX (1 << 3)
+#define NFQA_CFG_F_UID_GID (1 << 3)
+#define NFQA_CFG_F_MAX (1 << 4)
/* flags for NFQA_SKB_INFO */
/* packet appears to have wrong checksums, but they are ok */
diff --git a/src/libnetfilter_queue.c b/src/libnetfilter_queue.c
index fa8efe7..32725d1 100644
--- a/src/libnetfilter_queue.c
+++ b/src/libnetfilter_queue.c
@@ -668,6 +668,9 @@ EXPORT_SYMBOL(nfq_set_mode);
* if this bit is set, the layer 3/4 checksums of the packet appear incorrect,
* but are not (because they will be corrected later by the kernel).
*
+ * - NFQA_CFG_F_UID_GID: the kernel will dump UID and GID of the socket to
+ * which each packet belongs.
+ *
* Here's a little code snippet to show how to use this API:
* \verbatim
uint32_t flags = NFQA_CFG_F_FAIL_OPEN;
@@ -1181,6 +1184,38 @@ struct nfqnl_msg_packet_hw *nfq_get_packet_hw(struct nfq_data *nfad)
EXPORT_SYMBOL(nfq_get_packet_hw);
/**
+ * nfq_get_uid - get the UID of the user the packet belongs to
+ * \param nfad Netlink packet data handle passed to callback function
+ *
+ * \return 1 if there is a UID available, 0 otherwise.
+ */
+int nfq_get_uid(struct nfq_data *nfad, u_int32_t *uid)
+{
+ if (!nfnl_attr_present(nfad->data, NFQA_UID))
+ return 0;
+
+ *uid = ntohl(nfnl_get_data(nfad->data, NFQA_UID, u_int32_t));
+ return 1;
+}
+EXPORT_SYMBOL(nfq_get_uid);
+
+/**
+ * nfq_get_gid - get the GID of the user the packet belongs to
+ * \param nfad Netlink packet data handle passed to callback function
+ *
+ * \return 1 if there is a GID available, 0 otherwise.
+ */
+int nfq_get_gid(struct nfq_data *nfad, u_int32_t *gid)
+{
+ if (!nfnl_attr_present(nfad->data, NFQA_GID))
+ return 0;
+
+ *gid = ntohl(nfnl_get_data(nfad->data, NFQA_GID, u_int32_t));
+ return 1;
+}
+EXPORT_SYMBOL(nfq_get_gid);
+
+/**
* nfq_get_payload - get payload
* \param nfad Netlink packet data handle passed to callback function
* \param data Pointer of pointer that will be pointed to the payload
@@ -1250,6 +1285,7 @@ int nfq_snprintf_xml(char *buf, size_t rem, struct nfq_data *tb, int flags)
struct nfqnl_msg_packet_hdr *ph;
struct nfqnl_msg_packet_hw *hwph;
u_int32_t mark, ifi;
+ u_int32_t uid, gid;
int size, offset = 0, len = 0, ret;
unsigned char *data;
@@ -1365,6 +1401,16 @@ int nfq_snprintf_xml(char *buf, size_t rem, struct nfq_data *tb, int flags)
SNPRINTF_FAILURE(size, rem, offset, len);
}
+ if (nfq_get_uid(tb, &uid) && (flags & NFQ_XML_UID)) {
+ size = snprintf(buf + offset, rem, "<uid>%u</uid>", uid);
+ SNPRINTF_FAILURE(size, rem, offset, len);
+ }
+
+ if (nfq_get_gid(tb, &gid) && (flags & NFQ_XML_GID)) {
+ size = snprintf(buf + offset, rem, "<gid>%u</gid>", gid);
+ SNPRINTF_FAILURE(size, rem, offset, len);
+ }
+
ret = nfq_get_payload(tb, &data);
if (ret >= 0 && (flags & NFQ_XML_PAYLOAD)) {
int i;
diff --git a/src/nlmsg.c b/src/nlmsg.c
index e7a30e0..81e170e 100644
--- a/src/nlmsg.c
+++ b/src/nlmsg.c
@@ -134,6 +134,8 @@ static int nfq_pkt_parse_attr_cb(const struct nlattr *attr, void *data)
case NFQA_IFINDEX_PHYSOUTDEV:
case NFQA_CAP_LEN:
case NFQA_SKB_INFO:
+ case NFQA_UID:
+ case NFQA_GID:
if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
return MNL_CB_ERROR;
break;
--
1.8.5.1
^ permalink raw reply related
* Re: [PATCH net 2/2] net: core: explicitly select a txq before doing l2 forwarding
From: Neil Horman @ 2014-01-07 13:17 UTC (permalink / raw)
To: Jason Wang; +Cc: davem, netdev, linux-kernel, mst, John Fastabend, e1000-devel
In-Reply-To: <52CB77A0.3030106@redhat.com>
On Tue, Jan 07, 2014 at 11:42:24AM +0800, Jason Wang wrote:
> On 01/06/2014 08:42 PM, Neil Horman wrote:
> > On Mon, Jan 06, 2014 at 11:21:07AM +0800, Jason Wang wrote:
> >> Currently, the tx queue were selected implicitly in ndo_dfwd_start_xmit(). The
> >> will cause several issues:
> >>
> >> - NETIF_F_LLTX was forced for macvlan device in this case which lead extra lock
> >> contention.
> >> - dev_hard_start_xmit() was called with NULL txq which bypasses the net device
> >> watchdog
> >> - dev_hard_start_xmit() does not check txq everywhere which will lead a crash
> >> when tso is disabled for lower device.
> >>
> >> Fix this by explicitly introducing a select queue method just for l2 forwarding
> >> offload (ndo_dfwd_select_queue), and introducing dfwd_direct_xmit() to do the
> >> queue selecting and transmitting for l2 forwarding.
> >>
> >> With this fixes, NETIF_F_LLTX could be preserved for macvlan and there's no need
> >> to check txq against NULL in dev_hard_start_xmit().
> >>
> >> In the future, it was also required for macvtap l2 forwarding support since it
> >> provides a necessary synchronization method.
> >>
> >> Cc: John Fastabend <john.r.fastabend@intel.com>
> >> Cc: Neil Horman <nhorman@tuxdriver.com>
> >> Cc: e1000-devel@lists.sourceforge.net
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > Instead of creating another operation here to do special queue selection, why
> > not just have ndo_dfwd_start_xmit include a pointer to a pointer in its argument
> > list, so it can pass the txq it used back to the caller (dev_hard_start_xmit)?
> > ndo_dfwd_start_xmit already knows which queue set to pick from (since their
> > reserved for the device doing the transmitting). It seems more clear to me than
> > creating a new netdevice operation.
>
> See commit 8ffab51b3dfc54876f145f15b351c41f3f703195 ("macvlan: lockless
> tx path"). The point is keep the tx path lockless to be efficient and
> simplicity for management. And macvtap multiqueue was also implemented
> with this assumption. The real contention should be done in the txq of
> lower device instead of macvlan itself. This is also needed for
> multiqueue macvtap.
> >
Ok, I see how you're preserving LLTX here, and thats great, but it doesn't
really buy us anything that I can see. If a macvlan is using hardware
acceleration, it needs to arbitrate access to that hardware. Weather thats done
by locking the lowerdev's tx queue lock or by enforcing locking on the macvlan
itself is equivalent. The decision to use dfwd hardware acceleration is made on
open, so its not like theres any traffic that can avoid the lock, as it all goes
through the hardware. All I see that this has bought us is an extra net_device
method (which isn't a big deal, but not necessecary as I see it).
It seems like the right solution here is to use ethtool to disable the dfwd
acceleration feature on the hardware if you don't want to incur the additional
locking.
> > As for the crash issue, I'm not sure what you mean. Where in
> > dev_hard_start_xmit would we need to check txq that we're not currently, and
> > what crash results?
>
> Well, see current dev_hard_start_xmit(), if lower device does not
> support tso or tso is disabled, in gso path:
>
> gso:
> ...
> txq_trans_update(txq);
> if (unlikely(netif_xmit_stopped(txq) && skb->next))
>
> There's an obvious NULL pointer dereference.
Yeah, I saw that after I wrote my note, sorry about that. However, it doesn't
change what I said above. i don't think theres a need to add an additional
select_queue method here, just add a pointer to a pointer arg to the dfwd xmit
routine to fill in the queue that was selected on transmit. That should resolve
all the potential NULL pointers without the need to ad additional methods to
net_device_ops.
> >
> > Also, can you elaborate on what you mean by additional lock contention?
>
> If the lower device has NETIF_F_LLTX, then both macvlan txq lock and the
> lock of device itself must be held before doing transmission. In the
> case, the macvlan txq lock contention is obvious unnecessary.
Not in its current implementation. The lowerdevice's NETIF_F_LLTX are ignored
during transmit because we're using a privately allocated set of queues assigned
to the transmitting macvlan (i.e. there is not contention on the lowerdevice for
any single given macvlan). Thats why we have to clear the NETIF_F_LLTX field on
the macvlan itself. using dfwd acceleration is effectively giving a macvlan its
own hardware, that needs arbitration between every context thats trying to
transmit over it.
That said, I did just notice that if the macvlan has multiple queues we will
still use a single tx queue on transmit, we should map those queues to multiple
hardware queues. I'll fix that shortly.
> > What
> > contention do you see that goes above and beyond the normal locking required by
> > txq access?
>
> As I said above, the point is keeping the lockess tx path and make the
> contention of txq for real device instead of macvlan itself.
But you've not done that. You've just moved the locking down to the lowerdev,
which is fine I suppose, but doesn't actually improve anything, since we will
still lock exactly as many time as if we do the locking in the macvlan.
Regards
Neil
> > I suppose its extra locking above and beyond in the macvtap case,
> > where you would otherwise never hit hardware, but that not the only use case,
> > and I think the solution there is likely to add some code in the macvlan feature
> > set handler so that NETIF_F_LLTX is cleared if you disable the hardware
> > forwarding acceleration via ethtool.
> >
> > Regards
> > Neil
> >
>
>
^ permalink raw reply
* Re: single process receives own frames due to PACKET_MMAP
From: Norbert van Bolhuis @ 2014-01-07 13:16 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Daniel Borkmann; +Cc: netdev, David Miller, uaca
In-Reply-To: <20140107110609.74f71979@redhat.com>
On 01/07/14 11:06, Jesper Dangaard Brouer wrote:
> On Tue, 07 Jan 2014 10:32:01 +0100
> Daniel Borkmann<dborkman@redhat.com> wrote:
>
>> On 01/06/2014 11:58 PM, Norbert van Bolhuis wrote:
>>>
>>> Our application uses raw AF_PACKET socket to send and receive
>>> on one particular ethernet interface.
>>>
>>> Recently we started using PACKET_MMAP (TPACKET_V2). This makes
>>> the Appl use a TX socket and a RX socket.
>>> Both sockets are bound to the same (eth) interface. I noticed
>>> the RX socket receives all frames that are sent via the
>>> TX socket (same process, different thread). This I do not want.
>>>
>>> I know it is supposed to happen for different processes
>>> (otherwise wireshark won't work), but I did not expect it to
>>> happen for one single process (with different threads).
>>>
>>> I can filter them out in user-space (PACKET_OUTGOING)
>>> or via kernel packet filter (SO_ATTACH_FILTER), but performance is
>>> critical.
>>>
>>> I wonder whether this (PACKET_MMAP) behaviour is OK.
>>
>> For your use-case, we recently introduced in d346a3fae3ff1
>> ("packet: introduce PACKET_QDISC_BYPASS socket option") a
>> bypass of dev_queue_xmit() (that internally invokes
>> dev_queue_xmit_nit()).
>>
>>> It did not happen before (with a non-PACKET_MMAP AF_PACKET socket
>>> which was used by both threads of the same Appl process). So
>>> why is it happening now ?
>>
>> Can you elaborate a bit on which kernel versions that behaviour
>> changed?
Sorry, I wasn't very clear. This does not regard kernel (versions).
With "before" I mean our previous version of our application which
used an AF_PACKET socket, but not the PACKET_MMAP option.
>>
>>> I'd say it makes no sense to make the same process receive its
>>> own transmitted frames on that same interface (unless its lo).
>
> Have you setup:
> ring->s_ll.sll_protocol = 0
>
> This is what I did in trafgen to avoid this problem.
>
> See line 55 in netsniff-ng/ring.c:
> https://github.com/borkmann/netsniff-ng/blob/c3602a995b21e8133c7f4fd1fb1e7e21b6a844f1/ring.c#L55
>
> Commit:
> https://github.com/borkmann/netsniff-ng/commit/c3602a995b21e8133c7f4fd1fb1e7e21b6a844f1
>
No I did not do that, I was checking my code against netsniff-ng-0.5.8-rc4.
But I just tried it, I believe I do the same as netsniff-ng-0.5.8-rc5, but it doesn't
work for me. Maybe because I have an old FC14 system (kernel 2.6.35.14-106.fc14.x86_64).
So I tried to see whether netsniff-ng-0.5.8-rc5/trafgen still makes the
kernel call packet_rcv() on my FC14 system. So I build and run it, but I'm not sure
how to (easily) check that. In anyway, Wireshark does capture the trafgen generated
frames, does that say anything ?
In the future, I can at least use PACKET_QDISC_BYPASS as a "workaround".
Thanks a lot for your answers.
---
Norbert
^ permalink raw reply
* Re: [PATCH net-next v4] IPv6: use anycast addresses as source addresses in echo reply
From: Hannes Frederic Sowa @ 2014-01-07 13:09 UTC (permalink / raw)
To: François-Xavier Le Bail
Cc: netdev, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki Yoshifuji, Patrick McHardy
In-Reply-To: <1389098318.83283.YahooMailBasic@web125506.mail.ne1.yahoo.com>
On Tue, Jan 07, 2014 at 04:38:38AM -0800, François-Xavier Le Bail wrote:
> On Mon, 1/6/14, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
>
> >>But if an echo request arrive on an interface
> >> which do not listen at this LL anycast, is a reply can occur ?
>
> > I thought quite the opposite with anycast + forwarding
> > enabled, that we would process the packet.
>
> yes, for a global scope anycast.
> But a routers must not forward any packets with link-local source or destination addresses to other links, thus also ll anycast.
Of course, but in this case I hesitated because I haven't dealt a lot
with anycast recently in routing code and it seemed like RTF_ANYCAST is
exclusive-or with RTF_LOCAL. So I was really unsure if this protection
was really in place. We already had some illegal forwarding/input paths
in the past. ;)
Greetings,
Hannes
^ permalink raw reply
* [PATCH 2/2] net/mlx4_core: clean up srq_res_start_move_to()
From: Paul Bolle @ 2014-01-07 13:02 UTC (permalink / raw)
To: Or Gerlitz, Jack Morgenstein, Rony Efraim, Hadar Hen Zion,
David S. Miller
Cc: netdev, linux-kernel
Building resource_tracker.o triggers a GCC warning:
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function 'mlx4_HW2SW_SRQ_wrapper':
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:3202:17: warning: 'srq' may be used uninitialized in this function [-Wmaybe-uninitialized]
atomic_dec(&srq->mtt->ref_count);
^
This is a false positive. But a cleanup of srq_res_start_move_to() can
help GCC here. The code currently uses a switch statement where a plain
if/else would do, since only two of the switch's four cases can ever
occur. Dropping that switch makes the warning go away.
While we're at it, do some coding style cleanups (missing braces),
convert state to the correct type, and drop a test that always evaluates
to true.
Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
.../net/ethernet/mellanox/mlx4/resource_tracker.c | 45 ++++++++--------------
1 file changed, 16 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
index a41f01e..8ace450 100644
--- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
+++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
@@ -1372,7 +1372,7 @@ static int cq_res_start_move_to(struct mlx4_dev *dev, int slave, int cqn,
}
static int srq_res_start_move_to(struct mlx4_dev *dev, int slave, int index,
- enum res_cq_states state, struct res_srq **srq)
+ enum res_srq_states state, struct res_srq **srq)
{
struct mlx4_priv *priv = mlx4_priv(dev);
struct mlx4_resource_tracker *tracker = &priv->mfunc.master.res_tracker;
@@ -1381,39 +1381,26 @@ static int srq_res_start_move_to(struct mlx4_dev *dev, int slave, int index,
spin_lock_irq(mlx4_tlock(dev));
r = res_tracker_lookup(&tracker->res_tree[RES_SRQ], index);
- if (!r)
+ if (!r) {
err = -ENOENT;
- else if (r->com.owner != slave)
+ } else if (r->com.owner != slave) {
err = -EPERM;
- else {
- switch (state) {
- case RES_SRQ_BUSY:
+ } else if (state == RES_SRQ_ALLOCATED) {
+ if (r->com.state != RES_SRQ_HW)
err = -EINVAL;
- break;
-
- case RES_SRQ_ALLOCATED:
- if (r->com.state != RES_SRQ_HW)
- err = -EINVAL;
- else if (atomic_read(&r->ref_count))
- err = -EBUSY;
- break;
-
- case RES_SRQ_HW:
- if (r->com.state != RES_SRQ_ALLOCATED)
- err = -EINVAL;
- break;
-
- default:
+ else if (atomic_read(&r->ref_count))
+ err = -EBUSY;
+ } else {
+ /* state == RES_SRQ_HW */
+ if (r->com.state != RES_SRQ_ALLOCATED)
err = -EINVAL;
- }
+ }
- if (!err) {
- r->com.from_state = r->com.state;
- r->com.to_state = state;
- r->com.state = RES_SRQ_BUSY;
- if (srq)
- *srq = r;
- }
+ if (!err) {
+ r->com.from_state = r->com.state;
+ r->com.to_state = state;
+ r->com.state = RES_SRQ_BUSY;
+ *srq = r;
}
spin_unlock_irq(mlx4_tlock(dev));
--
1.8.4.2
^ permalink raw reply related
* [PATCH 1/2] net/mlx4_core: clean up cq_res_start_move_to()
From: Paul Bolle @ 2014-01-07 13:01 UTC (permalink / raw)
To: Or Gerlitz, Jack Morgenstein, Rony Efraim, Hadar Hen Zion,
David S. Miller
Cc: netdev, linux-kernel
Building resource_tracker.o triggers a GCC warning:
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function 'mlx4_HW2SW_CQ_wrapper':
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:3019:16: warning: 'cq' may be used uninitialized in this function [-Wmaybe-uninitialized]
atomic_dec(&cq->mtt->ref_count);
^
This is a false positive. But a cleanup of cq_res_start_move_to() can
help GCC here. The code currently uses a switch statement where a plain
if/else would do, since only two of the switch's four cases can ever
occur. Dropping that switch makes the warning go away.
While we're at it, do some coding style cleanups (missing braces), and
drop a test that always evaluates to true.
Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
.../net/ethernet/mellanox/mlx4/resource_tracker.c | 51 ++++++++--------------
1 file changed, 19 insertions(+), 32 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
index 2f3f2bc..a41f01e 100644
--- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
+++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
@@ -1340,43 +1340,30 @@ static int cq_res_start_move_to(struct mlx4_dev *dev, int slave, int cqn,
spin_lock_irq(mlx4_tlock(dev));
r = res_tracker_lookup(&tracker->res_tree[RES_CQ], cqn);
- if (!r)
+ if (!r) {
err = -ENOENT;
- else if (r->com.owner != slave)
+ } else if (r->com.owner != slave) {
err = -EPERM;
- else {
- switch (state) {
- case RES_CQ_BUSY:
+ } else if (state == RES_CQ_ALLOCATED) {
+ if (r->com.state != RES_CQ_HW)
+ err = -EINVAL;
+ else if (atomic_read(&r->ref_count))
err = -EBUSY;
- break;
-
- case RES_CQ_ALLOCATED:
- if (r->com.state != RES_CQ_HW)
- err = -EINVAL;
- else if (atomic_read(&r->ref_count))
- err = -EBUSY;
- else
- err = 0;
- break;
-
- case RES_CQ_HW:
- if (r->com.state != RES_CQ_ALLOCATED)
- err = -EINVAL;
- else
- err = 0;
- break;
-
- default:
+ else
+ err = 0;
+ } else {
+ /* state == RES_CQ_HW */
+ if (r->com.state != RES_CQ_ALLOCATED)
err = -EINVAL;
- }
+ else
+ err = 0;
+ }
- if (!err) {
- r->com.from_state = r->com.state;
- r->com.to_state = state;
- r->com.state = RES_CQ_BUSY;
- if (cq)
- *cq = r;
- }
+ if (!err) {
+ r->com.from_state = r->com.state;
+ r->com.to_state = state;
+ r->com.state = RES_CQ_BUSY;
+ *cq = r;
}
spin_unlock_irq(mlx4_tlock(dev));
--
1.8.4.2
^ permalink raw reply related
* Re: [PATCH net-next v4] IPv6: use anycast addresses as source addresses in echo reply
From: Hannes Frederic Sowa @ 2014-01-07 13:01 UTC (permalink / raw)
To: François-Xavier Le Bail
Cc: David Miller, netdev, kuznet, jmorris, yoshfuji, kaber
In-Reply-To: <1389098302.90053.YahooMailBasic@web125504.mail.ne1.yahoo.com>
On Tue, Jan 07, 2014 at 04:38:22AM -0800, François-Xavier Le Bail wrote:
> On Mon, 1/6/14, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
>
>
> > > > I think we should pass a valid device in unless it
> > > > breaks something obvious.
>
> > > The problem is that "saddr" is not necessarily an address on "skb->dev"
> > > in icmpv6_echo_reply(). It may be an address on another interface.
>
> > Maybe you're right, but then I don't get it. Could you make
> > an example?
>
> Yes :
> box with eth1, eth2, forwarding enable.
>
> echo request arrives on eth2
> -------------------------------> eth2 (2a01:3::1) {forwarding enable} eth1 (2a01:2::1)
>
> if dest == 2a01:3::
> Jan 7 10:36:36 localhost kernel: [ 59.155376] icmpv6_echo_reply: saddr == 2a01:3::
> Jan 7 10:36:36 localhost kernel: [ 59.155395] icmpv6_echo_reply: skb->dev->name == eth2
> Jan 7 10:36:36 localhost kernel: [ 59.155398] icmpv6_echo_reply: ipv6_chk_acast_addr(net, skb->dev, saddr) == 1
> Jan 7 10:36:36 localhost kernel: [ 59.155400] icmpv6_echo_reply: ipv6_chk_acast_addr(net, NULL, saddr) == 1
> if dest == 2a01:2::
> Jan 7 10:36:46 localhost kernel: [ 68.807565] icmpv6_echo_reply: saddr == 2a01:2::
> Jan 7 10:36:46 localhost kernel: [ 68.807580] icmpv6_echo_reply: skb->dev->name == eth2
> Jan 7 10:36:46 localhost kernel: [ 68.807583] icmpv6_echo_reply: ipv6_chk_acast_addr(net, skb->dev, saddr) == 0
> Jan 7 10:36:46 localhost kernel: [ 68.807586] icmpv6_echo_reply: ipv6_chk_acast_addr(net, NULL, saddr) == 1
>
> So, as 2a01:2:: is a address on eth1, ipv6_chk_acast_addr(net, skb->dev, saddr) with dev == eth2 return 0.
>
> It is the reason why I use dev == NULL.
>
> Do your tests show something different ?
I haven't tested this particular setup.
>
> Anyway, although I think that this solution is valid, I am testing another way to do this change.
Ok, thanks for explaining, I see now, of course.
Maybe we could just do (ipv6_addr_type(addr) & IPV6_ADDR_LINKLOCAL) ? skb->dev
: NULL?
I guess the NULL solution would be ok now, too. You can decide. I just
think we can be a bit more defensive here with no additional cost. Routing
table behaviour is pretty complicated and maybe can change in future.
Thank you,
Hannes
^ permalink raw reply
* [PATCH 0/2] net/mlx4_core: clean up two functions
From: Paul Bolle @ 2014-01-07 13:00 UTC (permalink / raw)
To: Or Gerlitz, Jack Morgenstein, Rony Efraim, Hadar Hen Zion,
David S. Miller
Cc: netdev, linux-kernel
0) These two patches are very similar. They both clean up a function to
help GCC understand the codeflow. Both help silence a warning.
1) Compile tested only (on 32 bits x86). I do not have this hardware.
2) Please note that there's no MAINTAINERS entry for mlx4_core. (Neither
is there an entry for the MLX4 IB driver.) Shouldn't it be added?
Paul Bolle (2):
net/mlx4_core: clean up cq_res_start_move_to()
net/mlx4_core: clean up srq_res_start_move_to()
.../net/ethernet/mellanox/mlx4/resource_tracker.c | 96 ++++++++--------------
1 file changed, 35 insertions(+), 61 deletions(-)
--
1.8.4.2
^ permalink raw reply
* Re: [RFC PATCH net-next 1/4] net: introduce backup_classid to struct skbuff
From: Libo Chen @ 2014-01-07 12:54 UTC (permalink / raw)
To: David Miller
Cc: edumazet-hpIqsD4AKlfQT0dZR+AlfA, jasowang-H+wXaHxf7aLQT0dZR+AlfA,
horms-/R6kz+dDXgpPR4JQBCEnsQ, serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA,
netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
kaber-dcUjhNyLwpNeoWH0uzbU5w, xemul-GEFAQzZX7r8dnm+yROfE0A,
ebiederm-aS9lmoZGLiVWk0Htik3J/w,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, jhs-jkUAjuhPggJWk0Htik3J/w,
lizefan-hv44wF8Li93QT0dZR+AlfA, Ding Tianhong
In-Reply-To: <20140103.003404.1439874071277993396.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On 2014/1/3 13:34, David Miller wrote:
> From: Libo Chen <clbchenlibo.chen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Date: Fri, 3 Jan 2014 11:11:04 +0800
>
>>
>> introduce backup_classid to struct skbuff,
>> we can use it to backup sk_classid when net_ns switch.
>>
>> Signed-off-by: Libo Chen <clbchenlibo.chen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>
> Sorry, no new sk_buff members unless there is absolutely not other
> possible implementation.
>
I'm kind of banging my head against the wall with this,
but can not think of a better way.
skb_shared_info is the extension for sk_buffer, how about moving into it?
any comments are appreciated!
thanks,
Libo
> sk_buff is too big as-is.
>
> .
>
^ permalink raw reply
* Re: [PATCH net] bridge: Fix crash with vlan filtering and tcpdump
From: Toshiaki Makita @ 2014-01-07 12:52 UTC (permalink / raw)
To: vyasevic; +Cc: Toshiaki Makita, netdev
In-Reply-To: <52CAD39A.7040607@redhat.com>
On Mon, 2014-01-06 at 11:02 -0500, Vlad Yasevich wrote:
> On 01/05/2014 10:02 AM, Toshiaki Makita wrote:
> > On Fri, 2014-01-03 at 11:12 -0500, Vlad Yasevich wrote:
> >> When the vlan filtering is enabled on the bridge, but
> >> the filter is not configured on the bridge device itself,
> >> running tcpdump on the bridge device will result in a
> >> an Ooops with NULL pointer dereference. The reason
> > ^^^^^
> > Oops?
> >
> >> is that br_pass_frame_up() will bypass the vlan
> >> check because promisc flag is set. It will then try
> >> to get the table pointer and process the packet based
> >> on the table. Since the table pointer is NULL, we oops.
> >> Catch this special condition in br_handle_vlan().
> >>
> >> Reported-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> >> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >> ---
> > ...
> >> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> >> index af5ebd1..ea0c29b 100644
> >> --- a/net/bridge/br_vlan.c
> >> +++ b/net/bridge/br_vlan.c
> >> @@ -144,6 +144,18 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
> >> if (!br->vlan_enabled)
> >> goto out;
> >>
> >> + /* Vlan filter table must be configrured at this point. The
> >> + * only exception is the bridge is set in promisc mode and the
> >> + * packet is destined for the bridge device. In this case
> >> + * pass the packet as is.
> >> + */
> >> + if (!pv) {
> >> + if ((br->dev->flags & IFF_PROMISC) && skb->dev == br->dev)
> >> + goto out;
> >
> > Currently this function is calling __vlan_put_tag() if port_idx==0.
> > You are going to bypass it only if br is promisc mode.
> >
> > However it doesn't seem to lead any problem because
> > __netif_receive_skb_core() can handle hw accel vlan tag.
> >
> > Instead, I doubt if __vlan_put_tag() is necessary in br_handle_vlan()...
>
> I'll need to re-test, but I remember having had to add that code to make
> it work on the bridge...
>
> Looking at it again, I agree that special handling for forwarding to the
> bridge doesn't seem to be needed.
>
Then, I'm OK with that "goto" statement :)
One more comment:
How about using BUG_ON() instead of "if-else BUG()"?
I think it could make the code slightly more efficient and simple.
Thanks,
Toshiaki Makita
^ permalink raw reply
* Re: [PATCH net-next v4] IPv6: use anycast addresses as source addresses in echo reply
From: François-Xavier Le Bail @ 2014-01-07 12:38 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: netdev, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki Yoshifuji, Patrick McHardy
On Mon, 1/6/14, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
>>But if an echo request arrive on an interface
>> which do not listen at this LL anycast, is a reply can occur ?
> I thought quite the opposite with anycast + forwarding
> enabled, that we would process the packet.
yes, for a global scope anycast.
But a routers must not forward any packets with link-local source or destination addresses to other links, thus also ll anycast.
BR
François-Xavier
^ permalink raw reply
* Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr
From: Toshiaki Makita @ 2014-01-07 12:42 UTC (permalink / raw)
To: vyasevic; +Cc: Toshiaki Makita, David S . Miller, Stephen Hemminger, netdev
In-Reply-To: <52CA9398.3090603@redhat.com>
On Mon, 2014-01-06 at 06:29 -0500, Vlad Yasevich wrote:
> On 01/05/2014 10:26 AM, Toshiaki Makita wrote:
> > On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote:
> >> On 01/03/2014 02:28 PM, Vlad Yasevich wrote:
> >>> On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> >>>> br_fdb_changeaddr() assumes that there is at most one local entry per port
> >>>> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
> >>>> creating/deleting fdb entries via netlink"), it has not been so.
> >>>> Therefore, the function might fail to search a correct previous address
> >>>> to be deleted and delete an arbitrary local entry if user has added local
> >>>> entries manually.
> >>>>
> >>>> Example of problematic case:
> >>>> ip link set eth0 address ee:ff:12:34:56:78
> >>>> brctl addif br0 eth0
> >>>> bridge fdb add 12:34:56:78:90:ab dev eth0 master
> >>>> ip link set eth0 address aa:bb:cc:dd:ee:ff
> >>>> Then, the address 12:34:56:78:90:ab might be deleted instead of
> >>>> ee:ff:12:34:56:78, the original mac address of eth0.
> >>>>
> >>>> Address this issue by introducing a new flag, added_by_user, to struct
> >>>> net_bridge_fdb_entry.
> >>>>
> >>>> Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case
> >>>> like:
> >>>> ip link set eth0 address 12:34:56:78:90:ab
> >>>> ip link set eth1 address aa:bb:cc:dd:ee:ff
> >>>> brctl addif br0 eth0
> >>>> bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master
> >>>> brctl addif br0 eth1
> >>>> brctl delif br0 eth0
> >>>> In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff,
> >>>> but it also should have been added by "brctl addif br0 eth1" originally,
> >>>> so we don't delete it and treat it a new kernel-created entry.
> >>>>
> >>>
> >>> I was looking over my patch series that adds something similar to this
> >>> and noticed that you are not handing the NTF_USE case. That case was
> >>> always troublesome for me as it allows for 2 different way to create
> >>> the same FDB: one through br_fdb_update() and one through fdb_add_entry().
> >>>
> >>> It is possible, though I haven't found any users yet, that NTF_USE
> >>> may be used and in that case, bridge will create a dynamic fdb and
> >>> disregard all NUD flags. In case case, add_by_user will not be set
> >>> either.
> >>>
> >>> I think that the above is broken and plan to submit a fix shortly.
> >>
> >> Just looked again at my NTF_USE patch and while it seems ok, the whole
> >> NTF_USE usage is racy to begin with and I am really starting to question
> >> it's validity.
> >>
> >> Presently, br_fdb_update() will not update local fdb entries. Instead
> >> it will log a misleading warning... It will only let you update
> >> non-local entries. This is fine for user-created entries, but any
> >> operation on dynamically created entries will only persist until
> >> the next packet. It also races against the packet, so there is
> >> absolutely no guarantee that the values of fdb->dst and fdb->updated
> >> will be consistent..
> >>
> >> It seems to me that the update capability of NTF_USE would actually be
> >> of more value on local or user-created fdb entries.
> >>
> >> The fdb creation capability of NTF_USE should be disabled.
> >>
> >> Thoughts?
> >
> > I ignored NTF_USE in this patch because I regard it as emulating kernel
> > creating entries after investigating git log.
> >
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498
> >
> > So I think NTF_USE shouldn't set added_by_user.
> > And to emulate kernel creating entries, simply calling br_fdb_update()
> > is the right way, isn't it?
>
> You can create dynamic entries (emulating the kernel) without NTF_USE.
> Just set the NUD_REACHABLE. Notice that arp cache only uses NTF_USE
> to trigger and arp notification. The creation is still triggered via
> other netlink flags.
>
> The more I look at this the more I think NTF_USE should not create
> an entry all by itself.
I haven't fully understood you yet.
Currently NTF_USE behaves as if the port receives a frame and it seems
to work, though the ability to create entries is different from neigh
subsystem.
Why do you want to change the behavior?
Are you worried about inconsistency of NLM-flags/NUD-state with NTF_USE
between neigh and bridge?
Thanks,
Toshiaki Makita
^ permalink raw reply
* Re: [PATCH net-next v4] IPv6: use anycast addresses as source addresses in echo reply
From: François-Xavier Le Bail @ 2014-01-07 12:38 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: David Miller, netdev, kuznet, jmorris, yoshfuji, kaber
On Mon, 1/6/14, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
> > > I think we should pass a valid device in unless it
> > > breaks something obvious.
> > The problem is that "saddr" is not necessarily an address on "skb->dev"
> > in icmpv6_echo_reply(). It may be an address on another interface.
> Maybe you're right, but then I don't get it. Could you make
> an example?
Yes :
box with eth1, eth2, forwarding enable.
echo request arrives on eth2
-------------------------------> eth2 (2a01:3::1) {forwarding enable} eth1 (2a01:2::1)
if dest == 2a01:3::
Jan 7 10:36:36 localhost kernel: [ 59.155376] icmpv6_echo_reply: saddr == 2a01:3::
Jan 7 10:36:36 localhost kernel: [ 59.155395] icmpv6_echo_reply: skb->dev->name == eth2
Jan 7 10:36:36 localhost kernel: [ 59.155398] icmpv6_echo_reply: ipv6_chk_acast_addr(net, skb->dev, saddr) == 1
Jan 7 10:36:36 localhost kernel: [ 59.155400] icmpv6_echo_reply: ipv6_chk_acast_addr(net, NULL, saddr) == 1
if dest == 2a01:2::
Jan 7 10:36:46 localhost kernel: [ 68.807565] icmpv6_echo_reply: saddr == 2a01:2::
Jan 7 10:36:46 localhost kernel: [ 68.807580] icmpv6_echo_reply: skb->dev->name == eth2
Jan 7 10:36:46 localhost kernel: [ 68.807583] icmpv6_echo_reply: ipv6_chk_acast_addr(net, skb->dev, saddr) == 0
Jan 7 10:36:46 localhost kernel: [ 68.807586] icmpv6_echo_reply: ipv6_chk_acast_addr(net, NULL, saddr) == 1
So, as 2a01:2:: is a address on eth1, ipv6_chk_acast_addr(net, skb->dev, saddr) with dev == eth2 return 0.
It is the reason why I use dev == NULL.
Do your tests show something different ?
Anyway, although I think that this solution is valid, I am testing another way to do this change.
BR,
Francois-Xavier
^ permalink raw reply
* RE: [PATCH net-next v2 6/6] r8152: support RTL8153
From: hayeswang @ 2014-01-07 12:35 UTC (permalink / raw)
To: 'Bjørn Mork'
Cc: oliver, netdev, 'nic_swsd', linux-kernel, linux-usb
In-Reply-To: <87eh4l31or.fsf@nemi.mork.no>
Bjørn Mork [mailto:bjorn@mork.no]
> Sent: Monday, January 06, 2014 5:22 PM
> To: Hayeswang
> Cc: oliver@neukum.org; netdev@vger.kernel.org; nic_swsd;
> linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org
> Subject: Re: [PATCH net-next v2 6/6] r8152: support RTL8153
[...]
> Exactly the same device, but now cfg #1 is active and a
> different set of
> drivers have bound to the interfaces. This is possible
> because none of
> the involved drivers disable the support for this device at
> build-time.
> Instead they use the available interface descriptors for matching and
> probing supported functions.
>
> End users will of course normally not go around writing stuff to sysfs
> attributes like this. Creating an udev rule to select a specific
> counfiguration when the device is plugged is more useful for normal
> usage.
Thanks for your answer. I would study udev rule first.
Does the udev alwayes exist for all Linux system, such as
Android, embedded system, and so on?
Best Regards,
Hayes
^ permalink raw reply
* Re: [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes
From: Hannes Frederic Sowa @ 2014-01-07 12:22 UTC (permalink / raw)
To: Thomas Haller; +Cc: Jiri Pirko, netdev, stephen, dcbw
In-Reply-To: <1389096874.2248.7.camel@weing>
Hi Thomas!
On Tue, Jan 07, 2014 at 01:14:34PM +0100, Thomas Haller wrote:
> I am about to resent the patch, so that IFA_F_NOPREFIXROUTE is saved in
> the flags of the address. Later, when deleting such address, this is
> used to indicate ~not~ to delete any prefix route... just as you suggest
> in your earlier email (if I understood you right).
Ok, good.
> About this suggestion now, I tend to "no". Yes, it could be sensible, on
> the other hand, if user space already controls the routes (as indicated
> by it's use of IFA_F_NOPREFIXROUTES), I would just leave it to the user
> to clean up the wrong prefix route.
>
> What do you think?
It is a matter of usability. As prefix routes are added to main routing
table and are visible via simple ip -6 r l (and not ip -6 r l table
something), I am fine with that, too.
Greetings,
Hannes
^ permalink raw reply
* Re: [PATCH v2 00/16] wl1251 patches from linux-n900 tree
From: Pavel Machek @ 2014-01-07 12:15 UTC (permalink / raw)
To: John W. Linville
Cc: Pali Rohár, Luciano Coelho, linux-wireless, netdev,
linux-kernel, freemangordon, aaro.koskinen, sre, joni.lapilainen,
Johannes Berg, Felipe Contreras
In-Reply-To: <20140106200050.GG10885@tuxdriver.com>
Hi!
> > > Second version contains new patch for fixing NULL pointer
> > > dereference which sometimes cause kernel panic and fixes code
> > > suggested by Pavel Machek.
>
> > So far, there are no new comments for patches 01, 03-13 and 16.
> > Are there any other problems with that patches or can be accepted
> > for upstream?
>
> They are probably fine to merge now. Of course, I was still deleting
> wl12xx patches when they were originally posted, so I'll need someone
> to resend the patches to me...
Patches should be in your inbox now, with subjects "[PATCH v3 13/13]
wl1251:". I hand selected patches 01, 03-13 and 16. I was doing
forwarding by hand, sorry for extra (fwd) in 06/13.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes
From: Thomas Haller @ 2014-01-07 12:14 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: Jiri Pirko, netdev, stephen, dcbw
In-Reply-To: <20140107120111.GB24730@order.stressinduktion.org>
[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]
On Tue, 2014-01-07 at 13:01 +0100, Hannes Frederic Sowa wrote:
> On Mon, Jan 06, 2014 at 06:29:35PM +0100, Thomas Haller wrote:
> > @@ -3662,8 +3666,10 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags,
> > if (!(ifp->flags&IFA_F_TENTATIVE))
> > ipv6_ifa_notify(0, ifp);
> >
> > - addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
> > - expires, flags);
> > + if (ifa_flags & IFA_F_NOPREFIXROUTE == 0) {
> > + addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
> > + expires, flags);
> > + }
>
> Actually, if we switch from IFA_F_NOPREFIXROUTE to !IFA_F_NOPREFIXROUTE we
> have to remove the prefix route, no?
>
> Greetings,
>
> Hannes
>
hi Hannes,
I am about to resent the patch, so that IFA_F_NOPREFIXROUTE is saved in
the flags of the address. Later, when deleting such address, this is
used to indicate ~not~ to delete any prefix route... just as you suggest
in your earlier email (if I understood you right).
About this suggestion now, I tend to "no". Yes, it could be sensible, on
the other hand, if user space already controls the routes (as indicated
by it's use of IFA_F_NOPREFIXROUTES), I would just leave it to the user
to clean up the wrong prefix route.
What do you think?
Thomas
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes
From: Hannes Frederic Sowa @ 2014-01-07 12:01 UTC (permalink / raw)
To: Thomas Haller; +Cc: Jiri Pirko, netdev, stephen, dcbw
In-Reply-To: <1389029375-17698-1-git-send-email-thaller@redhat.com>
On Mon, Jan 06, 2014 at 06:29:35PM +0100, Thomas Haller wrote:
> @@ -3662,8 +3666,10 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags,
> if (!(ifp->flags&IFA_F_TENTATIVE))
> ipv6_ifa_notify(0, ifp);
>
> - addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
> - expires, flags);
> + if (ifa_flags & IFA_F_NOPREFIXROUTE == 0) {
> + addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
> + expires, flags);
> + }
Actually, if we switch from IFA_F_NOPREFIXROUTE to !IFA_F_NOPREFIXROUTE we
have to remove the prefix route, no?
Greetings,
Hannes
^ permalink raw reply
* Re: [PATCH] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get
From: Vasily Averin @ 2014-01-07 11:42 UTC (permalink / raw)
To: netfilter-devel
Cc: Andrey Vagin, netfilter, coreteam, netdev, linux-kernel, vvs,
Florian Westphal, Pablo Neira Ayuso, Patrick McHardy,
Jozsef Kadlecsik, David S. Miller, Cyrill Gorcunov
In-Reply-To: <1389090711-15843-1-git-send-email-avagin@openvz.org>
On 01/07/2014 02:31 PM, Andrey Vagin wrote:
> Lets look at destroy_conntrack:
>
> hlist_nulls_del_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode);
> ...
> nf_conntrack_free(ct)
> kmem_cache_free(net->ct.nf_conntrack_cachep, ct);
>
> net->ct.nf_conntrack_cachep is created with SLAB_DESTROY_BY_RCU.
>
> The hash is protected by rcu, so readers look up conntracks without
> locks.
> A conntrack is removed from the hash, but in this moment a few readers
> still can use the conntrack. Then this conntrack is released and another
> thread creates conntrack with the same address and the equal tuple.
> After this a reader starts to validate the conntrack:
> * It's not dying, because a new conntrack was created
> * nf_ct_tuple_equal() returns true.
>
> But this conntrack is not initialized yet, so it can not be used by two
> threads concurrently. In this case BUG_ON may be triggered from
> nf_nat_setup_info().
>
> Florian Westphal suggested to check the confirm bit too. I think it's
> right.
>
> task 1 task 2 task 3
> nf_conntrack_find_get
> ____nf_conntrack_find
> destroy_conntrack
> hlist_nulls_del_rcu
> nf_conntrack_free
> kmem_cache_free
> __nf_conntrack_alloc
> kmem_cache_alloc
> memset(&ct->tuplehash[IP_CT_DIR_MAX],
> if (nf_ct_is_dying(ct))
> if (!nf_ct_tuple_equal()
>
> I'm not sure, that I have ever seen this race condition in a real life.
> Currently we are investigating a bug, which is reproduced on a few node.
> In our case one conntrack is initialized from a few tasks concurrently,
> we don't have any other explanation for this.
I would add; this strange traffic is generated by botnet clients (/etc/atdd and /boot/.IptabLes)
Several threads are send lot of identical tcp packets via RAW socket.
https://bugzilla.openvz.org/show_bug.cgi?id=2851
> <2>[46267.083061] kernel BUG at net/ipv4/netfilter/nf_nat_core.c:322!
> ...
> <4>[46267.083951] RIP: 0010:[<ffffffffa01e00a4>] [<ffffffffa01e00a4>] nf_nat_setup_info+0x564/0x590 [nf_nat]
> ...
> <4>[46267.085549] Call Trace:
> <4>[46267.085622] [<ffffffffa023421b>] alloc_null_binding+0x5b/0xa0 [iptable_nat]
> <4>[46267.085697] [<ffffffffa02342bc>] nf_nat_rule_find+0x5c/0x80 [iptable_nat]
> <4>[46267.085770] [<ffffffffa0234521>] nf_nat_fn+0x111/0x260 [iptable_nat]
> <4>[46267.085843] [<ffffffffa0234798>] nf_nat_out+0x48/0xd0 [iptable_nat]
> <4>[46267.085919] [<ffffffff814841b9>] nf_iterate+0x69/0xb0
> <4>[46267.085991] [<ffffffff81494e70>] ? ip_finish_output+0x0/0x2f0
> <4>[46267.086063] [<ffffffff81484374>] nf_hook_slow+0x74/0x110
> <4>[46267.086133] [<ffffffff81494e70>] ? ip_finish_output+0x0/0x2f0
> <4>[46267.086207] [<ffffffff814b5890>] ? dst_output+0x0/0x20
> <4>[46267.086277] [<ffffffff81495204>] ip_output+0xa4/0xc0
> <4>[46267.086346] [<ffffffff814b65a4>] raw_sendmsg+0x8b4/0x910
> <4>[46267.086419] [<ffffffff814c10fa>] inet_sendmsg+0x4a/0xb0
> <4>[46267.086491] [<ffffffff814459aa>] ? sock_update_classid+0x3a/0x50
> <4>[46267.086562] [<ffffffff81444d67>] sock_sendmsg+0x117/0x140
> <4>[46267.086638] [<ffffffff8151997b>] ? _spin_unlock_bh+0x1b/0x20
> <4>[46267.086712] [<ffffffff8109d370>] ? autoremove_wake_function+0x0/0x40
> <4>[46267.086785] [<ffffffff81495e80>] ? do_ip_setsockopt+0x90/0xd80
> <4>[46267.086858] [<ffffffff8100be0e>] ? call_function_interrupt+0xe/0x20
> <4>[46267.086936] [<ffffffff8118cb10>] ? ub_slab_ptr+0x20/0x90
> <4>[46267.087006] [<ffffffff8118cb10>] ? ub_slab_ptr+0x20/0x90
> <4>[46267.087081] [<ffffffff8118f2e8>] ? kmem_cache_alloc+0xd8/0x1e0
> <4>[46267.087151] [<ffffffff81445599>] sys_sendto+0x139/0x190
> <4>[46267.087229] [<ffffffff81448c0d>] ? sock_setsockopt+0x16d/0x6f0
> <4>[46267.087303] [<ffffffff810efa47>] ? audit_syscall_entry+0x1d7/0x200
> <4>[46267.087378] [<ffffffff810ef795>] ? __audit_syscall_exit+0x265/0x290
> <4>[46267.087454] [<ffffffff81474885>] ? compat_sys_setsockopt+0x75/0x210
> <4>[46267.087531] [<ffffffff81474b5f>] compat_sys_socketcall+0x13f/0x210
> <4>[46267.087607] [<ffffffff8104dea3>] ia32_sysret+0x0/0x5
> <4>[46267.087676] Code: 91 20 e2 01 75 29 48 89 de 4c 89 f7 e8 56 fa ff ff 85 c0 0f 84 68 fc ff ff 0f b6 4d c6 41 8b 45 00 e9 4d fb ff ff e8 7c 19 e9 e0 <0f> 0b eb fe f6 05 17 91 20 e2 80 74 ce 80 3d 5f 2e 00 00 00 74
> <1>[46267.088023] RIP [<ffffffffa01e00a4>] nf_nat_setup_info+0x564/0x590
>
> Cc: Florian Westphal <fw@strlen.de>
> Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> Cc: Patrick McHardy <kaber@trash.net>
> Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Signed-off-by: Andrey Vagin <avagin@openvz.org>
> ---
> net/netfilter/nf_conntrack_core.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 43549eb..7a34bb2 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -387,8 +387,12 @@ begin:
> !atomic_inc_not_zero(&ct->ct_general.use)))
> h = NULL;
> else {
> + /* A conntrack can be recreated with the equal tuple,
> + * so we need to check that the conntrack is initialized
> + */
> if (unlikely(!nf_ct_tuple_equal(tuple, &h->tuple) ||
> - nf_ct_zone(ct) != zone)) {
> + nf_ct_zone(ct) != zone) ||
> + !nf_ct_is_confirmed(ct)) {
> nf_ct_put(ct);
> goto begin;
> }
>
^ permalink raw reply
* [PATCH net-next 2/2] cxgb4: Changed FW check version to match FW binary version
From: Hariprasad Shenai @ 2014-01-07 11:28 UTC (permalink / raw)
To: netdev; +Cc: davem, dm, leedom, nirranjan, kumaras, santosh, hariprasad
In-Reply-To: <1389094088-6261-1-git-send-email-hariprasad@chelsio.com>
Signed-off-by: Hariprasad Shenai <hariprasad@chelsio.com>
---
drivers/net/ethernet/chelsio/cxgb4/cxgb4.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index 0d5b365..ddf7f8b 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -50,13 +50,13 @@
#include "cxgb4_uld.h"
#define T4FW_VERSION_MAJOR 0x01
-#define T4FW_VERSION_MINOR 0x06
-#define T4FW_VERSION_MICRO 0x18
+#define T4FW_VERSION_MINOR 0x09
+#define T4FW_VERSION_MICRO 0x17
#define T4FW_VERSION_BUILD 0x00
#define T5FW_VERSION_MAJOR 0x01
-#define T5FW_VERSION_MINOR 0x08
-#define T5FW_VERSION_MICRO 0x1C
+#define T5FW_VERSION_MINOR 0x09
+#define T5FW_VERSION_MICRO 0x17
#define T5FW_VERSION_BUILD 0x00
#define CH_WARN(adap, fmt, ...) dev_warn(adap->pdev_dev, fmt, ## __VA_ARGS__)
--
1.8.0
^ 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