| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 8 Sep 2020 12:38:14 -0400
From: Murali Karicheri <m-karicheri2@...com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC: David Miller <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Network Development <netdev@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>, <nsekhar@...com>,
Grygorii Strashko <grygorii.strashko@...com>
Subject: Re: [PATCH net-next 1/1] net: hsr/prp: add vlan support
Hi Willem,
Thanks for the response!
On 9/4/20 11:45 AM, Willem de Bruijn wrote:
> On Tue, Sep 1, 2020 at 9:54 PM Murali Karicheri <m-karicheri2@...com> wrote:
>>
>> This patch add support for creating vlan interfaces
>> over hsr/prp interface.
>>
>> Signed-off-by: Murali Karicheri <m-karicheri2@...com>
>> ---
>> net/hsr/hsr_device.c | 4 ----
>> net/hsr/hsr_forward.c | 16 +++++++++++++---
>> 2 files changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
>> index ab953a1a0d6c..e1951579a3ad 100644
>> --- a/net/hsr/hsr_device.c
>> +++ b/net/hsr/hsr_device.c
>> @@ -477,10 +477,6 @@ void hsr_dev_setup(struct net_device *dev)
>>
>> /* Prevent recursive tx locking */
>> dev->features |= NETIF_F_LLTX;
>> - /* VLAN on top of HSR needs testing and probably some work on
>> - * hsr_header_create() etc.
>> - */
>> - dev->features |= NETIF_F_VLAN_CHALLENGED;
>> /* Not sure about this. Taken from bridge code. netdev_features.h says
>> * it means "Does not change network namespaces".
>> */
>> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
>> index cadfccd7876e..de21df30b0d9 100644
>> --- a/net/hsr/hsr_forward.c
>> +++ b/net/hsr/hsr_forward.c
>> @@ -208,6 +208,7 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
>> struct hsr_port *port, u8 proto_version)
>> {
>> struct hsr_ethhdr *hsr_ethhdr;
>> + unsigned char *pc;
>> int lsdu_size;
>>
>> /* pad to minimum packet size which is 60 + 6 (HSR tag) */
>> @@ -218,7 +219,18 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
>> if (frame->is_vlan)
>> lsdu_size -= 4;
>>
>> - hsr_ethhdr = (struct hsr_ethhdr *)skb_mac_header(skb);
>> + pc = skb_mac_header(skb);
>> + if (frame->is_vlan)
>> + /* This 4-byte shift (size of a vlan tag) does not
>> + * mean that the ethhdr starts there. But rather it
>> + * provides the proper environment for accessing
>> + * the fields, such as hsr_tag etc., just like
>> + * when the vlan tag is not there. This is because
>> + * the hsr tag is after the vlan tag.
>> + */
>> + hsr_ethhdr = (struct hsr_ethhdr *)(pc + VLAN_HLEN);
>> + else
>> + hsr_ethhdr = (struct hsr_ethhdr *)pc;
>
> Instead, I would pass the header from the caller, which knows the
> offset because it moves the previous headers to make space.
>
So if I understood you correctly a diff for the above would like this
where pass dst + movelen as struct hsr_ethhdr *hsr_ethhdr
pointer to hsr_fill_tag(), right?
a0868495local@...0868495:~/Projects/upstream-kernel$ git diff
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index de21df30b0d9..4d9192c8bcf8 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -204,11 +204,10 @@ static void hsr_set_path_id(struct hsr_ethhdr
*hsr_ethhdr,
}
static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
+ struct hsr_ethhdr *hsr_ethhdr,
struct hsr_frame_info *frame,
struct hsr_port *port, u8
proto_version)
{
- struct hsr_ethhdr *hsr_ethhdr;
- unsigned char *pc;
int lsdu_size;
/* pad to minimum packet size which is 60 + 6 (HSR tag) */
@@ -219,19 +218,6 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff
*skb,
if (frame->is_vlan)
lsdu_size -= 4;
- pc = skb_mac_header(skb);
- if (frame->is_vlan)
- /* This 4-byte shift (size of a vlan tag) does not
- * mean that the ethhdr starts there. But rather it
- * provides the proper environment for accessing
- * the fields, such as hsr_tag etc., just like
- * when the vlan tag is not there. This is because
- * the hsr tag is after the vlan tag.
- */
- hsr_ethhdr = (struct hsr_ethhdr *)(pc + VLAN_HLEN);
- else
- hsr_ethhdr = (struct hsr_ethhdr *)pc;
-
hsr_set_path_id(hsr_ethhdr, port);
set_hsr_tag_LSDU_size(&hsr_ethhdr->hsr_tag, lsdu_size);
hsr_ethhdr->hsr_tag.sequence_nr = htons(frame->sequence_nr);
@@ -280,10 +266,12 @@ struct sk_buff *hsr_create_tagged_frame(struct
hsr_frame_info *frame,
memmove(dst, src, movelen);
skb_reset_mac_header(skb);
- /* skb_put_padto free skb on error and hsr_fill_tag returns NULL in
- * that case
+ /* dst point to the start of hsr tag. So pass it to fill the
+ * hsr info. Also skb_put_padto free skb on error and hsr_fill_tag
+ * returns NULL in that case.
*/
- return hsr_fill_tag(skb, frame, port, port->hsr->prot_version);
+ return hsr_fill_tag(skb, (struct hsr_ethhdr *)(dst + movelen),
+ frame, port, port->hsr->prot_version);
}
> Also, supporting VLAN probably also requires supporting 802.1ad QinQ,
> which means code should parse the headers instead of hardcoding
> VLAN_HLEN.
>
iec-62439-3 standard only talks about VLAN (TPID 0x8100), not about
QinQ. So what I could do is to check and bail out if 802.1ad frame is
received at the interface from upper layer. Something like below and
frame will get dropped.
@@ -519,6 +507,8 @@ static int fill_frame_info(struct hsr_frame_info *frame,
if (proto == htons(ETH_P_8021Q))
frame->is_vlan = true;
+ else if (proto == htons(ETH_P_8021AD))
+ return -1; /* Don't support 802.1ad */
if (frame->is_vlan) {
vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
What do you think?
Thanks.
>> hsr_set_path_id(hsr_ethhdr, port);
>> set_hsr_tag_LSDU_size(&hsr_ethhdr->hsr_tag, lsdu_size);
>> @@ -511,8 +523,6 @@ static int fill_frame_info(struct hsr_frame_info *frame,
>> if (frame->is_vlan) {
>> vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
>> proto = vlan_hdr->vlanhdr.h_vlan_encapsulated_proto;
>> - /* FIXME: */
>> - netdev_warn_once(skb->dev, "VLAN not yet supported");
>> }
>>
>> frame->is_from_san = false;
>> --
>> 2.17.1
>>
--
Murali Karicheri
Texas Instruments
Powered by blists - more mailing lists