Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v12] virtio-net: support inner header hash

2023-04-14 Thread Michael S. Tsirkin
On Fri, Apr 14, 2023 at 11:56:05AM +0800, Heng Qi wrote:
> 
> 
> 在 2023/4/14 上午5:43, Michael S. Tsirkin 写道:
> > On Thu, Apr 13, 2023 at 07:03:26PM +0800, Heng Qi wrote:
> > > > >    For example, when the packets of certain
> > > > > +tunnels are spread across multiple receive queues, these receive
> > > > > queues may have an unbalanced
> > > > > +amount of packets. This can cause a specific receive queue to
> > > > > become full, resulting in packet loss.
> > > > 
> > > > We have many places that can lead to packet dropping. For example, the
> > > > automatic steering is best effort. I tend to avoid mentioning things
> > > > like this.
> > > Ok. And Michael what do you think about this?
> > 
> > I think this text did not do a great job explaining the
> > security aspect. Here's a better, shorter explanation:
> > 
> > It is often an expectation of users that a tunnel isolates the external
> > network from the internal one. By completely ignoring entropy in the
> > external header and replacing it with entropy from the internal header,
> > for hash calculations, this expectation might be violated to a certain
> > extent, depending on how the hash is used. When the hash use is limited
> > to RSS queue selection, the effect will likely be limited to ability of
> > users inside the tunnel to cause packet drops in multiple queues (as
> > opposed to a single queue without the feature).
> 
> Sure. Will do  in the v13.
> 
> > 
> > > > 
> > > > > +
> > > > > +Possible mitigations:
> > > > > +\begin{itemize}
> > > > > +\item Use a tool with good forwarding performance to keep the
> > > > > receive queue from filling up.
> > > > > +\item If the QoS is unavailable, the driver can set
> > > > > \field{hash_tunnel_types} to VIRTIO_NET_HASH_TUNNEL_TYPE_NONE
> > > > > +  to disable inner header hash for encapsulated packets.
> > > > > +\item Choose a hash key that can avoid queue collisions.
> > > > > +\item Perform appropriate QoS before packets consume the receive
> > > > > buffers of the receive queues.
> > > > > +\end{itemize}
> > > > > +
> > > > > +The limitations mentioned above exist with/without the inner header
> > > > > hash.
> > > > 
> > > > This conflicts with the tile "Tunnel QoS limitation" which readers may
> > > > think it happens only for tunnel.
> > > Perhaps a "QoS Advices" is better?
> > Plural of "advice" is "advice" not "advices".
> 
> My fault.
> 
> > 
> > This advice is somewhat bogus though.
> > 
> > The point I keep trying to make is that this:
> > 
> > Choose a hash key that can avoid queue collisions.
> > 
> > is impossible with the feature and possible without.
> 
> I don't think so, the outer headers also has corresponding entropy for
> different streams.

But the feature when enabled ignores this entropy.

> Thanks.
> 
> > This was the whole reason I asked for a security
> > considerations sections.
> > 
> > 
> > > Thanks!
> > > 
> > > > Thanks
> > > > 
> > > > 
> > > > This publicly archived list offers a means to provide input to the
> > > > OASIS Virtual I/O Device (VIRTIO) TC.
> > > > 
> > > > In order to verify user consent to the Feedback License terms and
> > > > to minimize spam in the list archive, subscription is required
> > > > before posting.
> > > > 
> > > > Subscribe: virtio-comment-subscr...@lists.oasis-open.org
> > > > Unsubscribe: virtio-comment-unsubscr...@lists.oasis-open.org
> > > > List help: virtio-comment-h...@lists.oasis-open.org
> > > > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > > > Feedback License: 
> > > > https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > > > List Guidelines:
> > > > https://www.oasis-open.org/policies-guidelines/mailing-lists
> > > > Committee: https://www.oasis-open.org/committees/virtio/
> > > > Join OASIS: https://www.oasis-open.org/join/
> > 
> > -
> > To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



RE: [virtio-dev] Re: [virtio-comment] Re: [PATCH v12] virtio-net: support inner header hash

2023-04-13 Thread Parav Pandit


> From: Heng Qi 
> Sent: Friday, April 14, 2023 12:01 AM
> 
> 在 2023/4/14 上午11:10, Jason Wang 写道:
> > On Fri, Apr 14, 2023 at 5:46 AM Michael S. Tsirkin  wrote:
> >> On Thu, Apr 13, 2023 at 07:03:26PM +0800, Heng Qi wrote:
> >For example, when the packets of certain
> > +tunnels are spread across multiple receive queues, these receive
> > queues may have an unbalanced
> > +amount of packets. This can cause a specific receive queue to
> > become full, resulting in packet loss.
> 
>  We have many places that can lead to packet dropping. For example,
>  the automatic steering is best effort. I tend to avoid mentioning
>  things like this.
> >>> Ok. And Michael what do you think about this?
> >>
> >> I think this text did not do a great job explaining the security
> >> aspect. Here's a better, shorter explanation:
> >>
> >>  It is often an expectation of users that a tunnel isolates the 
> >> external
> >>  network from the internal one. By completely ignoring entropy in 
> >> the
> >>  external header and replacing it with entropy from the internal 
> >> header,
> >>  for hash calculations, this expectation might be violated to a 
> >> certain
> >>  extent, depending on how the hash is used. When the hash use is
> limited
> >>  to RSS queue selection, the effect will likely be limited to 
> >> ability of
> >>  users inside the tunnel to cause packet drops in multiple queues 
> >> (as
> >>  opposed to a single queue without the feature).
> > And this is only for GRE-in-UDP? This makes me think if we should add
> > GRE support for the outer header like:
> >
> > https://docs.napatech.com/r/Feature-Set-N-ANL9/Hash-Key-Type-10-3-Tupl
> > e-GREv0
> 
> I think this is for tunneling protocols with specific flow fields, such as 
> GRE:key
> filed, NVGRE:FLOWID filed.
> 
> This requires us to make a requirement when calculating the hash of the
> tunnels when F_TUNNEL_HASH is not negotiated. It's a new work.
> 
Yes, I think Jason is saying the same to extend the outer header hashing for 
newer protocols where outer header entropy is available.


Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v12] virtio-net: support inner header hash

2023-04-13 Thread Heng Qi




在 2023/4/14 上午11:10, Jason Wang 写道:

On Fri, Apr 14, 2023 at 5:46 AM Michael S. Tsirkin  wrote:

On Thu, Apr 13, 2023 at 07:03:26PM +0800, Heng Qi wrote:

   For example, when the packets of certain
+tunnels are spread across multiple receive queues, these receive
queues may have an unbalanced
+amount of packets. This can cause a specific receive queue to
become full, resulting in packet loss.


We have many places that can lead to packet dropping. For example, the
automatic steering is best effort. I tend to avoid mentioning things
like this.

Ok. And Michael what do you think about this?


I think this text did not do a great job explaining the
security aspect. Here's a better, shorter explanation:

 It is often an expectation of users that a tunnel isolates the external
 network from the internal one. By completely ignoring entropy in the
 external header and replacing it with entropy from the internal header,
 for hash calculations, this expectation might be violated to a certain
 extent, depending on how the hash is used. When the hash use is limited
 to RSS queue selection, the effect will likely be limited to ability of
 users inside the tunnel to cause packet drops in multiple queues (as
 opposed to a single queue without the feature).

And this is only for GRE-in-UDP? This makes me think if we should add
GRE support for the outer header like:

https://docs.napatech.com/r/Feature-Set-N-ANL9/Hash-Key-Type-10-3-Tuple-GREv0


I think this is for tunneling protocols with specific flow fields, such 
as GRE:key filed, NVGRE:FLOWID filed.


This requires us to make a requirement when calculating the hash of the 
tunnels when F_TUNNEL_HASH is not negotiated. It's a new work.


Thanks.



Thanks





+
+Possible mitigations:
+\begin{itemize}
+\item Use a tool with good forwarding performance to keep the
receive queue from filling up.
+\item If the QoS is unavailable, the driver can set
\field{hash_tunnel_types} to VIRTIO_NET_HASH_TUNNEL_TYPE_NONE
+  to disable inner header hash for encapsulated packets.
+\item Choose a hash key that can avoid queue collisions.
+\item Perform appropriate QoS before packets consume the receive
buffers of the receive queues.
+\end{itemize}
+
+The limitations mentioned above exist with/without the inner header
hash.


This conflicts with the tile "Tunnel QoS limitation" which readers may
think it happens only for tunnel.

Perhaps a "QoS Advices" is better?

Plural of "advice" is "advice" not "advices".

This advice is somewhat bogus though.

The point I keep trying to make is that this:

 Choose a hash key that can avoid queue collisions.

is impossible with the feature and possible without.
This was the whole reason I asked for a security
considerations sections.

--
MST



-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v12] virtio-net: support inner header hash

2023-04-13 Thread Heng Qi




在 2023/4/14 上午5:43, Michael S. Tsirkin 写道:

On Thu, Apr 13, 2023 at 07:03:26PM +0800, Heng Qi wrote:

   For example, when the packets of certain
+tunnels are spread across multiple receive queues, these receive
queues may have an unbalanced
+amount of packets. This can cause a specific receive queue to
become full, resulting in packet loss.


We have many places that can lead to packet dropping. For example, the
automatic steering is best effort. I tend to avoid mentioning things
like this.

Ok. And Michael what do you think about this?


I think this text did not do a great job explaining the
security aspect. Here's a better, shorter explanation:

It is often an expectation of users that a tunnel isolates the external
network from the internal one. By completely ignoring entropy in the
external header and replacing it with entropy from the internal header,
for hash calculations, this expectation might be violated to a certain
extent, depending on how the hash is used. When the hash use is limited
to RSS queue selection, the effect will likely be limited to ability of
users inside the tunnel to cause packet drops in multiple queues (as
opposed to a single queue without the feature).


Sure. Will do  in the v13.






+
+Possible mitigations:
+\begin{itemize}
+\item Use a tool with good forwarding performance to keep the
receive queue from filling up.
+\item If the QoS is unavailable, the driver can set
\field{hash_tunnel_types} to VIRTIO_NET_HASH_TUNNEL_TYPE_NONE
+  to disable inner header hash for encapsulated packets.
+\item Choose a hash key that can avoid queue collisions.
+\item Perform appropriate QoS before packets consume the receive
buffers of the receive queues.
+\end{itemize}
+
+The limitations mentioned above exist with/without the inner header
hash.


This conflicts with the tile "Tunnel QoS limitation" which readers may
think it happens only for tunnel.

Perhaps a "QoS Advices" is better?

Plural of "advice" is "advice" not "advices".


My fault.



This advice is somewhat bogus though.

The point I keep trying to make is that this:

Choose a hash key that can avoid queue collisions.

is impossible with the feature and possible without.


I don't think so, the outer headers also has corresponding entropy for 
different streams.


Thanks.


This was the whole reason I asked for a security
considerations sections.



Thanks!


Thanks


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscr...@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscr...@lists.oasis-open.org
List help: virtio-comment-h...@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines:
https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org