| Message ID | c7f44c08104958c86c68d98f1e093d754b0ca61b.1475672569.git.jbenc@redhat.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi, On Wed, Oct 5, 2016 at 4:07 PM, Jiri Benc <jbenc@redhat.com> wrote: > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c > index 4d67ea856067..c47b3da8ecf2 100644 > --- a/net/openvswitch/datapath.c > +++ b/net/openvswitch/datapath.c > @@ -594,6 +594,16 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info) > else > packet->protocol = htons(ETH_P_802_2); > > + if (eth_type_vlan(packet->protocol)) { > + __skb_pull(packet, ETH_HLEN); > + skb_reset_network_header(packet); > + skb_reset_mac_len(packet); > + packet = skb_vlan_untag(packet); > + if (unlikely(!packet)) > + goto err; I think at this point, 'eth' may point to a freed packet. Maybe replace the 'eth' variable with a 'proto' variable caching the value of eth_hdr(packet)->h_proto? > + skb_push(packet, ETH_HLEN); > + } > + > /* Set packet's mru */ > if (a[OVS_PACKET_ATTR_MRU]) { > mru = nla_get_u16(a[OVS_PACKET_ATTR_MRU]); > -- > 1.8.3.1 > Eyal.
On Wed, 5 Oct 2016 17:18:08 +0300, Eyal Birger wrote:
> I think at this point, 'eth' may point to a freed packet.
It may but how does that matter? eth is not used beyond that point.
Jiri
On Wed, Oct 5, 2016 at 8:23 PM, Jiri Benc <jbenc@redhat.com> wrote: > On Wed, 5 Oct 2016 17:18:08 +0300, Eyal Birger wrote: >> I think at this point, 'eth' may point to a freed packet. > > It may but how does that matter? eth is not used beyond that point. Definitely a nit. For sure not critical. Just seemed less future safe to keep a pointer to an old packet lying around. Eyal.
On Wed, Oct 05, 2016 at 08:31:52PM +0300, Eyal Birger wrote: > On Wed, Oct 5, 2016 at 8:23 PM, Jiri Benc <jbenc@redhat.com> wrote: > > On Wed, 5 Oct 2016 17:18:08 +0300, Eyal Birger wrote: > >> I think at this point, 'eth' may point to a freed packet. > > > > It may but how does that matter? eth is not used beyond that point. > > Definitely a nit. For sure not critical. > > Just seemed less future safe to keep a pointer to an old packet lying around. I agree. Alternatively refresh the eth pointer.
On Wed, 5 Oct 2016 14:44:26 -0400, Eric Garver wrote: > On Wed, Oct 05, 2016 at 08:31:52PM +0300, Eyal Birger wrote: > > Just seemed less future safe to keep a pointer to an old packet lying around. > > I agree. Alternatively refresh the eth pointer. Sorry guys, that just doesn't make sense. Everyone should know that reloading of skb pointer means the former pointers to its data may become invalid. Please point me to any place in the kernel where we reload the data pointer "just because" even when not used. Jiri
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c index 4d67ea856067..c47b3da8ecf2 100644 --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c @@ -594,6 +594,16 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info) else packet->protocol = htons(ETH_P_802_2); + if (eth_type_vlan(packet->protocol)) { + __skb_pull(packet, ETH_HLEN); + skb_reset_network_header(packet); + skb_reset_mac_len(packet); + packet = skb_vlan_untag(packet); + if (unlikely(!packet)) + goto err; + skb_push(packet, ETH_HLEN); + } + /* Set packet's mru */ if (a[OVS_PACKET_ATTR_MRU]) { mru = nla_get_u16(a[OVS_PACKET_ATTR_MRU]);
Similarly to how the core networking stack behaves, let the first vlan tag be always stored in skb->vlan_tci. This is already ensured in __netif_receive_skb_core for packets that were received from the kernel and honored by skb_vlan_push and skb_vlan_pop. The only remaining place are packets received from the user space. Just do the same things with vlan frames as __netif_receive_skb_core does. Signed-off-by: Jiri Benc <jbenc@redhat.com> --- net/openvswitch/datapath.c | 10 ++++++++++ 1 file changed, 10 insertions(+)