Hi Arend,
On 01/03/2018 02:24 PM, Arend Van Spriel wrote:
On Tue, Jan 2, 2018 at 2:27 PM, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
On Fri, 2017-12-29 at 12:29 -0600, Denis Kenzior wrote:
Agreed, requiring both attributes is less than ideal, but I tried to
make the initial RFC as minimal as possible. It also helped that iwd
uses SOCKET_OWNER by default. What can be done is to always set
conn_owner_nlportid and introduce another flag that would indicate
whether 'connection tear-down on application exit' was requested.
However, my opinion is that the current SOCKET_OWNER behavior should
just be made default, especially for control port over nl80211
connections, even if SOCKET_OWNER was not requested. Once the
controlling application dies, there's no hope of salvaging the
connection, perform rekeys, etc.
I think we should keep both attributes; it's better to be explicit that
both are needed than to set socket-owner automatically.
As I see it the problem is that SOCKET_OWNER behavior is actually two
behaviors wrapped in one, ie. 1) make notifications unicast, and 2)
tear-down on socket disconnect. So what is the reason for requiring
this attribute. In the cover letter you mention it is for unicast
notifications, but above you seem to put more weight in the tear-down
behavior. From earlier discussion I recall that multicast netlink
messages may not be received. Is that the reason for opting for
unicast here. If so, I would suggest to mention that in the commit
message.
There are several ways I could have implemented EAPoL over NL80211
notifications:
1. Multicast it to the wide world
2. Setup a registration framework ala CMD_REGISTER_FRAME or
CMD_UNEXPECTED_FRAME
3. Assume frames should be unicast to the process that initiated CMD_CONNECT
I see no good reason why anyone would want option 1. Option 2 seemed
pointless. Only the supplicant daemon (e.g. iwd or wpa_s) would really
want to process these anyway. Is this the reasoning you want me to
include in the cover letter?
The simplest way of accomplishing 3 was to reuse the conn_owner_nlportid
member that the kernel stores in the case that SOCKET_OWNER attribute is
used. iwd always uses this by default and quite frankly in the case of
EAPoL over NL80211 it doesn't make sense to not use it, at least to me.
Without SOCKET_OWNER, if the supplicant daemon dies you have an
un-managed, dangling connection.
The biggest issue was that each driver defines a set of management
frames it can accept via this mechanism.
I'm not sure this is "the biggest issue", but I tend to agree with
keeping them separate.
Yes. Seems like dealing with mgmt and data adds quite a bit of
complexity and/or redesign.
Just another note. In the cover letter it is mentioned that
eapol-over-nl80211 solves some long standing races. Now the example
mentioned is indeed one for which wpa_supplicant temporarily stores M1
yes, this race has to be taken care of by the supplicant. We have
similar workaround in iwd which I would love to get rid of.
or at least that is what I suspect you are referring to. Another
issue, for which we have a hack in brcmfmac, is a race between
installing the keys through nl80211 and M4 being passed through the
netdev which can result in M4 being sent out encrypted. Not sure if
your discussion about DONT_CRYPT is about that.
This is really a kernel issue being bled out into userspace. Many
drivers seem to suffer from this race. I think Ben had a thread some
time in June about this as well. But yes, I believe the main impetus to
add a DONT_ENCRYPT flag for EAPoL frames sent over NL80211 is to fix
this along with supporting weird protocols that explicitly want to be
sent unencrypted.
Regards,
-Denis
