[PATCH v2 net 0/4] bridge: Fix problems around the PVID

[PATCH v2 net 0/4] bridge: Fix problems around the PVID

[Toshiaki Makita]

There seem to be some undesirable behaviors related with PVID.
1. It has no effect assigning PVID to a port. PVID cannot be applied
to any frame regardless of whether we set it or not.
2. FDB entries learned via frames applied PVID are registered with
VID 0 rather than VID value of PVID.
3. We can set 0 or 4095 as a PVID that are not allowed in IEEE 802.1Q.
This leads interoperational problems such as sending frames with VID
4095, which is not allowed in IEEE 802.1Q, and treating frames with VID
0 as they belong to VLAN 0, which is expected to be handled as they have
no VID according to IEEE 802.1Q.

Note: 2nd and 3rd problems are potential and not exposed unless 1st problem
is fixed, because we cannot activate PVID due to it.

This is my analysis for each behavior.
1. We are using VLAN_TAG_PRESENT bit when getting PVID, and not when
adding/deleting PVID.
It can be fixed in either way using or not using VLAN_TAG_PRESENT,
but I think the latter is slightly more efficient.

2. We are setting skb->vlan_tci with the value of PVID but the variable
vid, which is used in FDB later, is set to 0 at br_allowed_ingress()
when untagged frames arrive at a port with PVID valid. I'm afraid that
vid should be updated to the value of PVID if PVID is valid.

3. According to IEEE 802.1Q-2011 (6.9.1 and Table 9-2), we cannot use
VID 0 or 4095 as a PVID.
It looks like that there are more stuff to consider.

- VID 0:
VID 0 shall not be configured in any FDB entry and used in a tag header
to indicate it is a 802.1p priority-tagged frame.
Priority-tagged frames should be applied PVID (from IEEE 802.1Q 6.9.1).
In my opinion, since we can filter incomming priority-tagged frames by
deleting PVID, we don't need to filter them by vlan_bitmap.
In other words, priority-tagged frames don't have VID 0 but have no VID,
which is the same as untagged frames, and should be filtered by unsetting
PVID.
So, not only we cannot set PVID as 0, but also we don't need to add 0 to
vlan_bitmap, which enables us to simply forbid to add vlan 0.

- VID 4095:
VID 4095 shall not be transmitted in a tag header. This VID value may be
used to indicate a wildcard match for the VID in management operations or
FDB entries (from IEEE 802.1Q Table 9-2).
In current implementation, we can create a static FDB entry with all
existing VIDs by not specifying any VID when creating it.
I don't think this way to add wildcard-like entries needs to change,
and VID 4095 looks no use and can be unacceptable to add.

Consequently, I believe what we should do for 3rd problem is below:
- Not allowing VID 0 and 4095 to be added.
- Applying PVID to priority-tagged (VID 0) frames.

Note: It has been descovered that another problem related to priority-tags
remains. If we use vlan 0 interface such as eth0.0, we cannot communicate
with another end station via a linux bridge.
This problem exists regardless of whether this patch set is applied or not
because we might receive untagged frames from another end station even if we
are sending priority-tagged frames.
This issue will be addressed by another patch set introducing an additional
egress policy, on which Vlad Yasevich is working.
See http://marc.info/?t=137880893800001&r=1&w=2 for detailed discussion.

Patch set follows this mail.
The order of patches is not the same as described above, because the way
to fix 1st problem is based on the assumption that we don't use VID 0 as
a PVID, which is realized by fixing 3rd problem.
(1/4)(2/4): Fix 3rd problem.
(3/4): Fix 1st problem.
(4/4): Fix 2nd probelm.

v2:
- Add descriptions about the problem related to priority-tags in cover letter.
- Revise patch comments to reference the newest spec.

Toshiaki Makita (4):
  bridge: Don't use VID 0 and 4095 in vlan filtering
  bridge: Apply the PVID to priority-tagged frames
  bridge: Fix the way the PVID is referenced
  bridge: Fix updating FDB entries when the PVID is applied

 net/bridge/br_fdb.c     |   4 +-
 net/bridge/br_netlink.c |   2 +-
 net/bridge/br_private.h |   4 +-
 net/bridge/br_vlan.c    | 125 ++++++++++++++++++++++++++----------------------
 4 files changed, 71 insertions(+), 64 deletions(-)

-- 
1.8.1.2

[PATCH v2 net 1/4] bridge: Don't use VID 0 and 4095 in vlan filtering

[Toshiaki Makita]

IEEE 802.1Q says that:
- VID 0 shall not be configured as a PVID, or configured in any Filtering
Database entry.
- VID 4095 shall not be configured as a PVID, or transmitted in a tag
header. This VID value may be used to indicate a wildcard match for the VID
in management operations or Filtering Database entries.
(See IEEE 802.1Q-2011 6.9.1 and Table 9-2)

Don't accept adding these VIDs in the vlan_filtering implementation.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Reviewed-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_fdb.c     |  4 +-
 net/bridge/br_netlink.c |  2 +-
 net/bridge/br_vlan.c    | 97 +++++++++++++++++++++++--------------------------
 3 files changed, 49 insertions(+), 54 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index ffd5874..33e8f23 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -700,7 +700,7 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 
 		vid = nla_get_u16(tb[NDA_VLAN]);
 
-		if (vid >= VLAN_N_VID) {
+		if (!vid || vid >= VLAN_VID_MASK) {
 			pr_info("bridge: RTM_NEWNEIGH with invalid vlan id %d\n",
 				vid);
 			return -EINVAL;
@@ -794,7 +794,7 @@ int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
 
 		vid = nla_get_u16(tb[NDA_VLAN]);
 
-		if (vid >= VLAN_N_VID) {
+		if (!vid || vid >= VLAN_VID_MASK) {
 			pr_info("bridge: RTM_NEWNEIGH with invalid vlan id %d\n",
 				vid);
 			return -EINVAL;
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index e74ddc1..f75d92e 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -243,7 +243,7 @@ static int br_afspec(struct net_bridge *br,
 
 		vinfo = nla_data(tb[IFLA_BRIDGE_VLAN_INFO]);
 
-		if (vinfo->vid >= VLAN_N_VID)
+		if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK)
 			return -EINVAL;
 
 		switch (cmd) {
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 9a9ffe7..21b6d21 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -45,37 +45,34 @@ static int __vlan_add(struct net_port_vlans *v, u16 vid, u16 flags)
 		return 0;
 	}
 
-	if (vid) {
-		if (v->port_idx) {
-			p = v->parent.port;
-			br = p->br;
-			dev = p->dev;
-		} else {
-			br = v->parent.br;
-			dev = br->dev;
-		}
-		ops = dev->netdev_ops;
-
-		if (p && (dev->features & NETIF_F_HW_VLAN_CTAG_FILTER)) {
-			/* Add VLAN to the device filter if it is supported.
-			 * Stricly speaking, this is not necessary now, since
-			 * devices are made promiscuous by the bridge, but if
-			 * that ever changes this code will allow tagged
-			 * traffic to enter the bridge.
-			 */
-			err = ops->ndo_vlan_rx_add_vid(dev, htons(ETH_P_8021Q),
-						       vid);
-			if (err)
-				return err;
-		}
-
-		err = br_fdb_insert(br, p, dev->dev_addr, vid);
-		if (err) {
-			br_err(br, "failed insert local address into bridge "
-			       "forwarding table\n");
-			goto out_filt;
-		}
+	if (v->port_idx) {
+		p = v->parent.port;
+		br = p->br;
+		dev = p->dev;
+	} else {
+		br = v->parent.br;
+		dev = br->dev;
+	}
+	ops = dev->netdev_ops;
+
+	if (p && (dev->features & NETIF_F_HW_VLAN_CTAG_FILTER)) {
+		/* Add VLAN to the device filter if it is supported.
+		 * Stricly speaking, this is not necessary now, since
+		 * devices are made promiscuous by the bridge, but if
+		 * that ever changes this code will allow tagged
+		 * traffic to enter the bridge.
+		 */
+		err = ops->ndo_vlan_rx_add_vid(dev, htons(ETH_P_8021Q),
+					       vid);
+		if (err)
+			return err;
+	}
 
+	err = br_fdb_insert(br, p, dev->dev_addr, vid);
+	if (err) {
+		br_err(br, "failed insert local address into bridge "
+		       "forwarding table\n");
+		goto out_filt;
 	}
 
 	set_bit(vid, v->vlan_bitmap);
@@ -98,7 +95,7 @@ static int __vlan_del(struct net_port_vlans *v, u16 vid)
 	__vlan_delete_pvid(v, vid);
 	clear_bit(vid, v->untagged_bitmap);
 
-	if (v->port_idx && vid) {
+	if (v->port_idx) {
 		struct net_device *dev = v->parent.port->dev;
 		const struct net_device_ops *ops = dev->netdev_ops;
 
@@ -248,7 +245,9 @@ bool br_allowed_egress(struct net_bridge *br,
 	return false;
 }
 
-/* Must be protected by RTNL */
+/* Must be protected by RTNL.
+ * Must be called with vid in range from 1 to 4094 inclusive.
+ */
 int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags)
 {
 	struct net_port_vlans *pv = NULL;
@@ -278,7 +277,9 @@ out:
 	return err;
 }
 
-/* Must be protected by RTNL */
+/* Must be protected by RTNL.
+ * Must be called with vid in range from 1 to 4094 inclusive.
+ */
 int br_vlan_delete(struct net_bridge *br, u16 vid)
 {
 	struct net_port_vlans *pv;
@@ -289,14 +290,9 @@ int br_vlan_delete(struct net_bridge *br, u16 vid)
 	if (!pv)
 		return -EINVAL;
 
-	if (vid) {
-		/* If the VID !=0 remove fdb for this vid. VID 0 is special
-		 * in that it's the default and is always there in the fdb.
-		 */
-		spin_lock_bh(&br->hash_lock);
-		fdb_delete_by_addr(br, br->dev->dev_addr, vid);
-		spin_unlock_bh(&br->hash_lock);
-	}
+	spin_lock_bh(&br->hash_lock);
+	fdb_delete_by_addr(br, br->dev->dev_addr, vid);
+	spin_unlock_bh(&br->hash_lock);
 
 	__vlan_del(pv, vid);
 	return 0;
@@ -329,7 +325,9 @@ unlock:
 	return 0;
 }
 
-/* Must be protected by RTNL */
+/* Must be protected by RTNL.
+ * Must be called with vid in range from 1 to 4094 inclusive.
+ */
 int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags)
 {
 	struct net_port_vlans *pv = NULL;
@@ -363,7 +361,9 @@ clean_up:
 	return err;
 }
 
-/* Must be protected by RTNL */
+/* Must be protected by RTNL.
+ * Must be called with vid in range from 1 to 4094 inclusive.
+ */
 int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 {
 	struct net_port_vlans *pv;
@@ -374,14 +374,9 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 	if (!pv)
 		return -EINVAL;
 
-	if (vid) {
-		/* If the VID !=0 remove fdb for this vid. VID 0 is special
-		 * in that it's the default and is always there in the fdb.
-		 */
-		spin_lock_bh(&port->br->hash_lock);
-		fdb_delete_by_addr(port->br, port->dev->dev_addr, vid);
-		spin_unlock_bh(&port->br->hash_lock);
-	}
+	spin_lock_bh(&port->br->hash_lock);
+	fdb_delete_by_addr(port->br, port->dev->dev_addr, vid);
+	spin_unlock_bh(&port->br->hash_lock);
 
 	return __vlan_del(pv, vid);
 }
-- 
1.8.1.2

Re: [PATCH v2 net 1/4] bridge: Don't use VID 0 and 4095 in vlan filtering

[Stephen Hemminger]

On Wed, 16 Oct 2013 17:07:13 +0900
Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:

> IEEE 802.1Q says that:
> - VID 0 shall not be configured as a PVID, or configured in any Filtering
> Database entry.
> - VID 4095 shall not be configured as a PVID, or transmitted in a tag
> header. This VID value may be used to indicate a wildcard match for the VID
> in management operations or Filtering Database entries.
> (See IEEE 802.1Q-2011 6.9.1 and Table 9-2)
> 
> Don't accept adding these VIDs in the vlan_filtering implementation.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Reviewed-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  net/bridge/br_fdb.c     |  4 +-
>  net/bridge/br_netlink.c |  2 +-
>  net/bridge/br_vlan.c    | 97 +++++++++++++++++++++++--------------------------
>  3 files changed, 49 insertions(+), 54 deletions(-)

This one looks good, thanks.

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH v2 net 2/4] bridge: Apply the PVID to priority-tagged frames

[Toshiaki Makita]

IEEE 802.1Q says that when we receive priority-tagged (VID 0) frames
use the PVID for the port as its VID.
(See IEEE 802.1Q-2011 6.9.1 and Table 9-2)

Apply the PVID to not only untagged frames but also priority-tagged frames.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_vlan.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 21b6d21..5a9c44a 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -189,6 +189,8 @@ out:
 bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
 			struct sk_buff *skb, u16 *vid)
 {
+	int err;
+
 	/* If VLAN filtering is disabled on the bridge, all packets are
 	 * permitted.
 	 */
@@ -201,20 +203,31 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
 	if (!v)
 		return false;
 
-	if (br_vlan_get_tag(skb, vid)) {
+	err = br_vlan_get_tag(skb, vid);
+	if (!*vid) {
 		u16 pvid = br_get_pvid(v);
 
-		/* Frame did not have a tag.  See if pvid is set
-		 * on this port.  That tells us which vlan untagged
-		 * traffic belongs to.
+		/* Frame had a tag with VID 0 or did not have a tag.
+		 * See if pvid is set on this port.  That tells us which
+		 * vlan untagged or priority-tagged traffic belongs to.
 		 */
 		if (pvid == VLAN_N_VID)
 			return false;
 
-		/* PVID is set on this port.  Any untagged ingress
-		 * frame is considered to belong to this vlan.
+		/* PVID is set on this port.  Any untagged or priority-tagged
+		 * ingress frame is considered to belong to this vlan.
 		 */
-		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid);
+		if (likely(err))
+			/* Untagged Frame. */
+			__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid);
+		else
+			/* Priority-tagged Frame.
+			 * At this point, We know that skb->vlan_tci had
+			 * VLAN_TAG_PRESENT bit and its VID field was 0x000.
+			 * We update only VID field and preserve PCP field.
+			 */
+			skb->vlan_tci |= pvid;
+
 		return true;
 	}
 
-- 
1.8.1.2

Re: [PATCH v2 net 2/4] bridge: Apply the PVID to priority-tagged frames

[Vlad Yasevich]

On 10/16/2013 04:07 AM, Toshiaki Makita wrote:
> IEEE 802.1Q says that when we receive priority-tagged (VID 0) frames
> use the PVID for the port as its VID.
> (See IEEE 802.1Q-2011 6.9.1 and Table 9-2)
>
> Apply the PVID to not only untagged frames but also priority-tagged frames.
>
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

Reviewed-by: Vlad Yasevich <vyasevic@redhat.com>

> ---
>   net/bridge/br_vlan.c | 27 ++++++++++++++++++++-------
>   1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 21b6d21..5a9c44a 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -189,6 +189,8 @@ out:
>   bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
>   			struct sk_buff *skb, u16 *vid)
>   {
> +	int err;
> +
>   	/* If VLAN filtering is disabled on the bridge, all packets are
>   	 * permitted.
>   	 */
> @@ -201,20 +203,31 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
>   	if (!v)
>   		return false;
>
> -	if (br_vlan_get_tag(skb, vid)) {
> +	err = br_vlan_get_tag(skb, vid);
> +	if (!*vid) {
>   		u16 pvid = br_get_pvid(v);
>
> -		/* Frame did not have a tag.  See if pvid is set
> -		 * on this port.  That tells us which vlan untagged
> -		 * traffic belongs to.
> +		/* Frame had a tag with VID 0 or did not have a tag.
> +		 * See if pvid is set on this port.  That tells us which
> +		 * vlan untagged or priority-tagged traffic belongs to.
>   		 */
>   		if (pvid == VLAN_N_VID)
>   			return false;
>
> -		/* PVID is set on this port.  Any untagged ingress
> -		 * frame is considered to belong to this vlan.
> +		/* PVID is set on this port.  Any untagged or priority-tagged
> +		 * ingress frame is considered to belong to this vlan.
>   		 */
> -		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid);
> +		if (likely(err))
> +			/* Untagged Frame. */
> +			__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid);
> +		else
> +			/* Priority-tagged Frame.
> +			 * At this point, We know that skb->vlan_tci had
> +			 * VLAN_TAG_PRESENT bit and its VID field was 0x000.
> +			 * We update only VID field and preserve PCP field.
> +			 */
> +			skb->vlan_tci |= pvid;
> +
>   		return true;
>   	}
>
>

Re: [PATCH v2 net 2/4] bridge: Apply the PVID to priority-tagged frames

[Stephen Hemminger]

On Wed, 16 Oct 2013 17:07:14 +0900
Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:

> IEEE 802.1Q says that when we receive priority-tagged (VID 0) frames
> use the PVID for the port as its VID.
> (See IEEE 802.1Q-2011 6.9.1 and Table 9-2)
> 
> Apply the PVID to not only untagged frames but also priority-tagged frames.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>  net/bridge/br_vlan.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 21b6d21..5a9c44a 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -189,6 +189,8 @@ out:
>  bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
>  			struct sk_buff *skb, u16 *vid)
>  {
> +	int err;
> +
>  	/* If VLAN filtering is disabled on the bridge, all packets are
>  	 * permitted.
>  	 */
> @@ -201,20 +203,31 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
>  	if (!v)
>  		return false;
>  
> -	if (br_vlan_get_tag(skb, vid)) {
> +	err = br_vlan_get_tag(skb, vid);
> +	if (!*vid) {
>  		u16 pvid = br_get_pvid(v);

Ok, but it looks like br_vlan_get_tag() could be cleaner if it just returned
the tag, and there was another br_vlan_tag_present() function.

Also, does this still work if CONFIG_BRIDGE_VLAN_FILTERING is disabled?

Re: [PATCH v2 net 2/4] bridge: Apply the PVID to priority-tagged frames

[Vlad Yasevich]

On 10/16/2013 11:55 AM, Stephen Hemminger wrote:
> On Wed, 16 Oct 2013 17:07:14 +0900
> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
>
>> IEEE 802.1Q says that when we receive priority-tagged (VID 0) frames
>> use the PVID for the port as its VID.
>> (See IEEE 802.1Q-2011 6.9.1 and Table 9-2)
>>
>> Apply the PVID to not only untagged frames but also priority-tagged frames.
>>
>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> ---
>>   net/bridge/br_vlan.c | 27 ++++++++++++++++++++-------
>>   1 file changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index 21b6d21..5a9c44a 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -189,6 +189,8 @@ out:
>>   bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
>>   			struct sk_buff *skb, u16 *vid)
>>   {
>> +	int err;
>> +
>>   	/* If VLAN filtering is disabled on the bridge, all packets are
>>   	 * permitted.
>>   	 */
>> @@ -201,20 +203,31 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
>>   	if (!v)
>>   		return false;
>>
>> -	if (br_vlan_get_tag(skb, vid)) {
>> +	err = br_vlan_get_tag(skb, vid);
>> +	if (!*vid) {
>>   		u16 pvid = br_get_pvid(v);
>
> Ok, but it looks like br_vlan_get_tag() could be cleaner if it just returned
> the tag, and there was another br_vlan_tag_present() function.

I was just thinking about that as well.  If we make br_vlan_get_tag()
return either the actual tag (if the packet is tagged), or the pvid
if (untagged/prio_tagged), then we can skp most of this.

>
> Also, does this still work if CONFIG_BRIDGE_VLAN_FILTERING is disabled?

Yes.  br_allowed_ingress becomes an inline if the config option is disabled.

-vlad

Re: [PATCH v2 net 2/4] bridge: Apply the PVID to priority-tagged frames

[Toshiaki Makita]

On Wed, 2013-10-16 at 12:16 -0400, Vlad Yasevich wrote:
> On 10/16/2013 11:55 AM, Stephen Hemminger wrote:
> > On Wed, 16 Oct 2013 17:07:14 +0900
> > Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
> >
> >> IEEE 802.1Q says that when we receive priority-tagged (VID 0) frames
> >> use the PVID for the port as its VID.
> >> (See IEEE 802.1Q-2011 6.9.1 and Table 9-2)
> >>
> >> Apply the PVID to not only untagged frames but also priority-tagged frames.
> >>
> >> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> >> ---
> >>   net/bridge/br_vlan.c | 27 ++++++++++++++++++++-------
> >>   1 file changed, 20 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> >> index 21b6d21..5a9c44a 100644
> >> --- a/net/bridge/br_vlan.c
> >> +++ b/net/bridge/br_vlan.c
> >> @@ -189,6 +189,8 @@ out:
> >>   bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
> >>   			struct sk_buff *skb, u16 *vid)
> >>   {
> >> +	int err;
> >> +
> >>   	/* If VLAN filtering is disabled on the bridge, all packets are
> >>   	 * permitted.
> >>   	 */
> >> @@ -201,20 +203,31 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
> >>   	if (!v)
> >>   		return false;
> >>
> >> -	if (br_vlan_get_tag(skb, vid)) {
> >> +	err = br_vlan_get_tag(skb, vid);
> >> +	if (!*vid) {
> >>   		u16 pvid = br_get_pvid(v);
> >
> > Ok, but it looks like br_vlan_get_tag() could be cleaner if it just returned
> > the tag, and there was another br_vlan_tag_present() function.

Thank you for reviewing.
I agree with you.
I had been afraid that if it affects other codes because
br_vlan_get_tag() is used in many places else, but now I have decided
not to hesitate to change its signature and behavior.

> 
> I was just thinking about that as well.  If we make br_vlan_get_tag()
> return either the actual tag (if the packet is tagged), or the pvid
> if (untagged/prio_tagged), then we can skp most of this.

Hmm... maybe I don't fully understand you.

Is what you intend something like
	br_allowed_ingress(...) {
		...
		vid = br_vlan_get_tag(skb, v);
		if (!tagged(skb)) put_tag(skb, vid); /* untagged */
		else if (!get_vid(skb)) update_vid(skb, vid); /* prio_tagged */
		...
	}

	br_vlan_get_tag(skb, v) {
		if (tagged(skb)) {
			vid = get_vid(skb);
			if (!vid) return get_pvid(v); /* prio_tagged */
			return vid;
		}
		return get_pvid(v); /* untagged */
	}

This needs double check for prio_tagged at br_allowed_ingress() and
br_vlan_get_tag().

Or if we modify skb->vlan_tci at br_vlan_get_tag(), isn't it a little
dangerous to other codes that use this function in order to just get
vid?

I am thinking it makes things simple that br_vlan_get_tag() returns 0 if
(untagged/prio_tagged).

	br_allowed_ingress(...) {
		...
		vid = br_vlan_get_tag(skb);
		if (!vid) {
			vid = get_pvid(v);
			if (!tagged(skb)) put_tag(skb, vid);/* untagged */
			else update_vid(skb, vid); /* prio_tagged */
		}
		...
	}

	br_vlan_get_tag(skb) {
		if (tagged(skb)) return get_vid(skb);
		return 0;
	}

Thanks,

Toshiaki Makita

> 
> >
> > Also, does this still work if CONFIG_BRIDGE_VLAN_FILTERING is disabled?
> 
> Yes.  br_allowed_ingress becomes an inline if the config option is disabled.
> 
> -vlad

Re: [PATCH v2 net 2/4] bridge: Apply the PVID to priority-tagged frames

[Vlad Yasevich]

On 10/17/2013 08:14 AM, Toshiaki Makita wrote:
> On Wed, 2013-10-16 at 12:16 -0400, Vlad Yasevich wrote:
>> On 10/16/2013 11:55 AM, Stephen Hemminger wrote:
>>> On Wed, 16 Oct 2013 17:07:14 +0900
>>> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
>>>
>>>> IEEE 802.1Q says that when we receive priority-tagged (VID 0) frames
>>>> use the PVID for the port as its VID.
>>>> (See IEEE 802.1Q-2011 6.9.1 and Table 9-2)
>>>>
>>>> Apply the PVID to not only untagged frames but also priority-tagged frames.
>>>>
>>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>>> ---
>>>>    net/bridge/br_vlan.c | 27 ++++++++++++++++++++-------
>>>>    1 file changed, 20 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>>>> index 21b6d21..5a9c44a 100644
>>>> --- a/net/bridge/br_vlan.c
>>>> +++ b/net/bridge/br_vlan.c
>>>> @@ -189,6 +189,8 @@ out:
>>>>    bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
>>>>    			struct sk_buff *skb, u16 *vid)
>>>>    {
>>>> +	int err;
>>>> +
>>>>    	/* If VLAN filtering is disabled on the bridge, all packets are
>>>>    	 * permitted.
>>>>    	 */
>>>> @@ -201,20 +203,31 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
>>>>    	if (!v)
>>>>    		return false;
>>>>
>>>> -	if (br_vlan_get_tag(skb, vid)) {
>>>> +	err = br_vlan_get_tag(skb, vid);
>>>> +	if (!*vid) {
>>>>    		u16 pvid = br_get_pvid(v);
>>>
>>> Ok, but it looks like br_vlan_get_tag() could be cleaner if it just returned
>>> the tag, and there was another br_vlan_tag_present() function.
>
> Thank you for reviewing.
> I agree with you.
> I had been afraid that if it affects other codes because
> br_vlan_get_tag() is used in many places else, but now I have decided
> not to hesitate to change its signature and behavior.
>
>>
>> I was just thinking about that as well.  If we make br_vlan_get_tag()
>> return either the actual tag (if the packet is tagged), or the pvid
>> if (untagged/prio_tagged), then we can skp most of this.
>
> Hmm... maybe I don't fully understand you.
>
> Is what you intend something like
> 	br_allowed_ingress(...) {
> 		...
> 		vid = br_vlan_get_tag(skb, v);
> 		if (!tagged(skb)) put_tag(skb, vid); /* untagged */
> 		else if (!get_vid(skb)) update_vid(skb, vid); /* prio_tagged */
> 		...
> 	}
>
> 	br_vlan_get_tag(skb, v) {
> 		if (tagged(skb)) {
> 			vid = get_vid(skb);
> 			if (!vid) return get_pvid(v); /* prio_tagged */
> 			return vid;
> 		}
> 		return get_pvid(v); /* untagged */
> 	}
>
> This needs double check for prio_tagged at br_allowed_ingress() and
> br_vlan_get_tag().
>
> Or if we modify skb->vlan_tci at br_vlan_get_tag(), isn't it a little
> dangerous to other codes that use this function in order to just get
> vid?
>
> I am thinking it makes things simple that br_vlan_get_tag() returns 0 if
> (untagged/prio_tagged).
>
> 	br_allowed_ingress(...) {
> 		...
> 		vid = br_vlan_get_tag(skb);
> 		if (!vid) {
> 			vid = get_pvid(v);
> 			if (!tagged(skb)) put_tag(skb, vid);/* untagged */
> 			else update_vid(skb, vid); /* prio_tagged */
> 		}
> 		...
> 	}
>
> 	br_vlan_get_tag(skb) {
> 		if (tagged(skb)) return get_vid(skb);
> 		return 0;
> 	}

With this you end up checking if the patcket is tagged quite a lot of times.

What I am thinking is that once we perform a get_tag, we should get
the vlan tag that the current packet belongs to.  We can then safely
use that tag everywhere and not have to worry too much about it.

We can pass that tag to br_allowed_ingress to verify that it is
permitted to enter.

You made a valid point about multicast code using br_vlan_get_tag
incorrectly and I plan on addressing that.

As it is, the current series addresses bugs in the implementation
that should be fixed.

We can make the code better/nicer as a next step.

-vlad

>
> Thanks,
>
> Toshiaki Makita
>
>>
>>>
>>> Also, does this still work if CONFIG_BRIDGE_VLAN_FILTERING is disabled?
>>
>> Yes.  br_allowed_ingress becomes an inline if the config option is disabled.
>>
>> -vlad
>
>

Re: [PATCH v2 net 2/4] bridge: Apply the PVID to priority-tagged frames

[Toshiaki Makita]

On Thu, 2013-10-17 at 13:39 -0400, Vlad Yasevich wrote:
> On 10/17/2013 08:14 AM, Toshiaki Makita wrote:
> > On Wed, 2013-10-16 at 12:16 -0400, Vlad Yasevich wrote:
> >> On 10/16/2013 11:55 AM, Stephen Hemminger wrote:
> >>> On Wed, 16 Oct 2013 17:07:14 +0900
> >>> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
> >>>
> >>>> IEEE 802.1Q says that when we receive priority-tagged (VID 0) frames
> >>>> use the PVID for the port as its VID.
> >>>> (See IEEE 802.1Q-2011 6.9.1 and Table 9-2)
> >>>>
> >>>> Apply the PVID to not only untagged frames but also priority-tagged frames.
> >>>>
> >>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> >>>> ---
> >>>>    net/bridge/br_vlan.c | 27 ++++++++++++++++++++-------
> >>>>    1 file changed, 20 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> >>>> index 21b6d21..5a9c44a 100644
> >>>> --- a/net/bridge/br_vlan.c
> >>>> +++ b/net/bridge/br_vlan.c
> >>>> @@ -189,6 +189,8 @@ out:
> >>>>    bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
> >>>>    			struct sk_buff *skb, u16 *vid)
> >>>>    {
> >>>> +	int err;
> >>>> +
> >>>>    	/* If VLAN filtering is disabled on the bridge, all packets are
> >>>>    	 * permitted.
> >>>>    	 */
> >>>> @@ -201,20 +203,31 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
> >>>>    	if (!v)
> >>>>    		return false;
> >>>>
> >>>> -	if (br_vlan_get_tag(skb, vid)) {
> >>>> +	err = br_vlan_get_tag(skb, vid);
> >>>> +	if (!*vid) {
> >>>>    		u16 pvid = br_get_pvid(v);
> >>>
> >>> Ok, but it looks like br_vlan_get_tag() could be cleaner if it just returned
> >>> the tag, and there was another br_vlan_tag_present() function.
> >
> > Thank you for reviewing.
> > I agree with you.
> > I had been afraid that if it affects other codes because
> > br_vlan_get_tag() is used in many places else, but now I have decided
> > not to hesitate to change its signature and behavior.
> >
> >>
> >> I was just thinking about that as well.  If we make br_vlan_get_tag()
> >> return either the actual tag (if the packet is tagged), or the pvid
> >> if (untagged/prio_tagged), then we can skp most of this.
> >
> > Hmm... maybe I don't fully understand you.
> >
> > Is what you intend something like
> > 	br_allowed_ingress(...) {
> > 		...
> > 		vid = br_vlan_get_tag(skb, v);
> > 		if (!tagged(skb)) put_tag(skb, vid); /* untagged */
> > 		else if (!get_vid(skb)) update_vid(skb, vid); /* prio_tagged */
> > 		...
> > 	}
> >
> > 	br_vlan_get_tag(skb, v) {
> > 		if (tagged(skb)) {
> > 			vid = get_vid(skb);
> > 			if (!vid) return get_pvid(v); /* prio_tagged */
> > 			return vid;
> > 		}
> > 		return get_pvid(v); /* untagged */
> > 	}
> >
> > This needs double check for prio_tagged at br_allowed_ingress() and
> > br_vlan_get_tag().
> >
> > Or if we modify skb->vlan_tci at br_vlan_get_tag(), isn't it a little
> > dangerous to other codes that use this function in order to just get
> > vid?
> >
> > I am thinking it makes things simple that br_vlan_get_tag() returns 0 if
> > (untagged/prio_tagged).
> >
> > 	br_allowed_ingress(...) {
> > 		...
> > 		vid = br_vlan_get_tag(skb);
> > 		if (!vid) {
> > 			vid = get_pvid(v);
> > 			if (!tagged(skb)) put_tag(skb, vid);/* untagged */
> > 			else update_vid(skb, vid); /* prio_tagged */
> > 		}
> > 		...
> > 	}
> >
> > 	br_vlan_get_tag(skb) {
> > 		if (tagged(skb)) return get_vid(skb);
> > 		return 0;
> > 	}
> 
> With this you end up checking if the patcket is tagged quite a lot of times.
> 
> What I am thinking is that once we perform a get_tag, we should get
> the vlan tag that the current packet belongs to.  We can then safely
> use that tag everywhere and not have to worry too much about it.
> 
> We can pass that tag to br_allowed_ingress to verify that it is
> permitted to enter.
> 
> You made a valid point about multicast code using br_vlan_get_tag
> incorrectly and I plan on addressing that.
> 
> As it is, the current series addresses bugs in the implementation
> that should be fixed.
> 
> We can make the code better/nicer as a next step.

OK, you seem to have a better idea to avoid checking if the packet is
tagged many times.

If this patch series is acceptable just as a bug fix, I'll wait for your
proposal of improvement and fixing wrong multicast codes next time.

Thanks,

Toshiaki Makita

> 
> -vlad
> 
> >
> > Thanks,
> >
> > Toshiaki Makita
> >
> >>
> >>>
> >>> Also, does this still work if CONFIG_BRIDGE_VLAN_FILTERING is disabled?
> >>
> >> Yes.  br_allowed_ingress becomes an inline if the config option is disabled.
> >>
> >> -vlad
> >
> >
> 

[PATCH v2 net 3/4] bridge: Fix the way the PVID is referenced

[Toshiaki Makita]

We are using the VLAN_TAG_PRESENT bit to detect whether the PVID is
set or not at br_get_pvid(), while we don't care about the bit in
adding/deleting the PVID, which makes it impossible to forward any
incomming untagged frame with vlan_filtering enabled.

Since vid 0 cannot be used for the PVID, we can use vid 0 to indicate
that the PVID is not set, which is slightly more efficient than using
the VLAN_TAG_PRESENT.

Fix the problem by getting rid of using the VLAN_TAG_PRESENT.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Reviewed-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_private.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index efb57d9..7ca2ae4 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -643,9 +643,7 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v)
 	 * vid wasn't set
 	 */
 	smp_rmb();
-	return (v->pvid & VLAN_TAG_PRESENT) ?
-			(v->pvid & ~VLAN_TAG_PRESENT) :
-			VLAN_N_VID;
+	return v->pvid ?: VLAN_N_VID;
 }
 
 #else
-- 
1.8.1.2

[PATCH v2 net 4/4] bridge: Fix updating FDB entries when the PVID is applied

[Toshiaki Makita]

We currently set the value that variable vid is pointing, which will be
used in FDB later, to 0 at br_allowed_ingress() when we receive untagged
or priority-tagged frames, even though the PVID is valid.
This leads to FDB updates in such a wrong way that they are learned with
VID 0.
Update the value to that of PVID if the PVID is applied.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Reviewed-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_vlan.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 5a9c44a..53f0990 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -217,6 +217,7 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
 		/* PVID is set on this port.  Any untagged or priority-tagged
 		 * ingress frame is considered to belong to this vlan.
 		 */
+		*vid = pvid;
 		if (likely(err))
 			/* Untagged Frame. */
 			__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid);
-- 
1.8.1.2

Re: [PATCH v2 net 4/4] bridge: Fix updating FDB entries when the PVID is applied

[Stephen Hemminger]

On Wed, 16 Oct 2013 17:07:16 +0900
Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:

> We currently set the value that variable vid is pointing, which will be
> used in FDB later, to 0 at br_allowed_ingress() when we receive untagged
> or priority-tagged frames, even though the PVID is valid.
> This leads to FDB updates in such a wrong way that they are learned with
> VID 0.
> Update the value to that of PVID if the PVID is applied.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Reviewed-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  net/bridge/br_vlan.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 5a9c44a..53f0990 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -217,6 +217,7 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
>  		/* PVID is set on this port.  Any untagged or priority-tagged
>  		 * ingress frame is considered to belong to this vlan.
>  		 */
> +		*vid = pvid;
>  		if (likely(err))
>  			/* Untagged Frame. */
>  			__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid);


Ok, but side-effects seem like an indication of poor code logic
flow design. Not your fault but part of the the per-vlan filtering code.

Re: [PATCH v2 net 4/4] bridge: Fix updating FDB entries when the PVID is applied

[Vlad Yasevich]

On 10/16/2013 11:57 AM, Stephen Hemminger wrote:
> On Wed, 16 Oct 2013 17:07:16 +0900
> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
>
>> We currently set the value that variable vid is pointing, which will be
>> used in FDB later, to 0 at br_allowed_ingress() when we receive untagged
>> or priority-tagged frames, even though the PVID is valid.
>> This leads to FDB updates in such a wrong way that they are learned with
>> VID 0.
>> Update the value to that of PVID if the PVID is applied.
>>
>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> Reviewed-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>>   net/bridge/br_vlan.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index 5a9c44a..53f0990 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -217,6 +217,7 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
>>   		/* PVID is set on this port.  Any untagged or priority-tagged
>>   		 * ingress frame is considered to belong to this vlan.
>>   		 */
>> +		*vid = pvid;
>>   		if (likely(err))
>>   			/* Untagged Frame. */
>>   			__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid);
>
>
> Ok, but side-effects seem like an indication of poor code logic
> flow design. Not your fault but part of the the per-vlan filtering code.
>

I'll see if I can re-work the code to get rid of the side-effects.

-vlad

Re: [PATCH v2 net 4/4] bridge: Fix updating FDB entries when the PVID is applied

[Toshiaki Makita]

On Wed, 2013-10-16 at 12:11 -0400, Vlad Yasevich wrote:
> On 10/16/2013 11:57 AM, Stephen Hemminger wrote:
> > On Wed, 16 Oct 2013 17:07:16 +0900
> > Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
> >
> >> We currently set the value that variable vid is pointing, which will be
> >> used in FDB later, to 0 at br_allowed_ingress() when we receive untagged
> >> or priority-tagged frames, even though the PVID is valid.
> >> This leads to FDB updates in such a wrong way that they are learned with
> >> VID 0.
> >> Update the value to that of PVID if the PVID is applied.
> >>
> >> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> >> Reviewed-by: Vlad Yasevich <vyasevic@redhat.com>
> >> ---
> >>   net/bridge/br_vlan.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> >> index 5a9c44a..53f0990 100644
> >> --- a/net/bridge/br_vlan.c
> >> +++ b/net/bridge/br_vlan.c
> >> @@ -217,6 +217,7 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
> >>   		/* PVID is set on this port.  Any untagged or priority-tagged
> >>   		 * ingress frame is considered to belong to this vlan.
> >>   		 */
> >> +		*vid = pvid;
> >>   		if (likely(err))
> >>   			/* Untagged Frame. */
> >>   			__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid);
> >
> >
> > Ok, but side-effects seem like an indication of poor code logic
> > flow design. Not your fault but part of the the per-vlan filtering code.
> >
> 
> I'll see if I can re-work the code to get rid of the side-effects.

I'm thinking br_allowed_ingress() might have too many roles.
- Determine whether an incoming frame is acceptable.
- Update skb->vlan_tci if PVID is applied.
- Update the argument 'vid'.

Besides, 'vid' is actually updated by not br_allowed_ingress() but
br_vlan_get_tag().

I think this complicated implementation could lead to missing expected
code for updating vid.

At least we can remove the third role from br_allowed_ingress() because
the required vid is recorded in skb->vlan_tci when we exit the function.

So, we can write the caller of br_allowed_ingress() like
	...
	if (!br_allowed_ingress(br, v, skb))
		goto drop;
	vid = br_vlan_get_tag(skb);

(Assuming br_vlan_get_tag() has been changed to return vid.)

However, this will require br_vlan_get_tag() to check br->vlan_enabled.
Does this change reduce complexity of current implementation?

BTW, some codes in mdb, such as br_multicast_ipv4_rcv(), seem to call
br_vlan_get_tag() without checking br->vlan_enabled.
Is this right way?
Or does br_vlan_get_tag() originally need to check br->vlan_enabled?

Thanks,

Toshiaki Makita

> 
> -vlad

Re: [PATCH v2 net 0/4] bridge: Fix problems around the PVID

[David Miller]

From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Date: Wed, 16 Oct 2013 17:07:12 +0900

> There seem to be some undesirable behaviors related with PVID.
> 1. It has no effect assigning PVID to a port. PVID cannot be applied
> to any frame regardless of whether we set it or not.
> 2. FDB entries learned via frames applied PVID are registered with
> VID 0 rather than VID value of PVID.
> 3. We can set 0 or 4095 as a PVID that are not allowed in IEEE 802.1Q.
> This leads interoperational problems such as sending frames with VID
> 4095, which is not allowed in IEEE 802.1Q, and treating frames with VID
> 0 as they belong to VLAN 0, which is expected to be handled as they have
> no VID according to IEEE 802.1Q.
 ...
> Patch set follows this mail.
> The order of patches is not the same as described above, because the way
> to fix 1st problem is based on the assumption that we don't use VID 0 as
> a PVID, which is realized by fixing 3rd problem.
> (1/4)(2/4): Fix 3rd problem.
> (3/4): Fix 1st problem.
> (4/4): Fix 2nd probelm.
> 
> v2:
> - Add descriptions about the problem related to priority-tags in cover letter.
> - Revise patch comments to reference the newest spec.

Series applied, thank you.