Hi Johannes,
> > +void sta_info_ipv4hash_add_sta(struct sta_info *sta,
> > + __be32 ipv4_addr) __acquires(RCU)
>
> > +int sta_info_ipv4hash_remove_sta(struct sta_info *sta)
> > +__acquires(RCU)
>
> Those sparse locking annotations clearly seem wrong - does that point to
> the annotations being superfluous or the implementation doing
> something else than you wanted?
They are wrong... these are leftovers from an advanced coding technique,
copy-and-paste. ;) Clearly, the lock is released before the exit, hence
those annotations should not be there.
> > +static inline int ieee80211_proxyarp_arp(struct
> ieee80211_sub_if_data *sdata,
> > + struct sk_buff *skb)
> > +{
> ...
> > + if (arp->ar_op == htons(ARPOP_REQUEST)) {
> > + struct sta_info *ssta, *tsta;
> > +
> > + /* Proxy ARP request for the STAs within the BSS */
> > + tsta = sta_info_get_ipv4(sdata, tip);
> > + if (tsta && !tsta->ipv4_lease_timeout &&
> > + time_after(jiffies, tsta->ipv4_lease_timeout)) {
> ...
> > + } else if (tsta && !ether_addr_equal(tsta->sta.addr, sha))
> {
> ...
> > + return 1;
> > + }
> ...
> > + /* Suppress ARP Request within the BSS */
> > + return 1;
> > +
>
> All code paths within the REQUEST return 1 - is that intentional? It seems
> to me that there might be a requirement to actually pass the frame if you
> don't know a response? OTOH, maybe you don't actually need this since
> you don't necessarily want the stations communicating anyway (at least
> in a HS2 scenario)
It is intentional for this particular case. There would be a case for
static IP, IPv4 DAD (RFC 5227), where a defense ARP Announcement packet
from the bridge would not only invalidate the table we built in the AP,
but also would inform the STA that there is an entity out there with
the same IP address. In this case, the "ARP REQUEST" frame would be
allowed through. Note: NOT implemented yet. This would be part 2.
The STAs not being able to communicate with each other, at least not
directly without the consent of an overseeing entity connected to the
bridge, is for the other feature in HS2, L2TIF (L2 Traffic Inspection
and Filtering). Coming to an e-mail inbox near you, hopefully in the
near future.
> > + } else if (arp->ar_op == htons(ARPOP_REPLY)) {
> > + if (is_multicast_ether_addr(eh->h_dest))
> > + return 1;
>
> is also implementing something else - not this particular feature, no?
Right. This is suppressing the G.ARP Reply within the BSS.
> Overall though, I can't say I like this. I can understand how it's very
> tempting to just stick all the code into the wireless stack and be done
> with it, but it's quite a bit of code that needs to parse all the frames etc.
> and I'm sure it will only get more complicated with the addition of IPv6.
> It's also not clear to me that parsing DHCP is actually the best course of
> action - if, for example, the DHCP server is colocated with the AP then it
> should be simple to actually listen to events coming from the DHCP
> server instead of piggy-backing on the actual frames.
> Additionally, even listening to those frames in userspace shouldn't be
> more complicated than a simple packet socket (with appropriate BPF.)
> Similar for ARP (which you seem to be using for some hash table updates)
> of course.
>
> If we assume then that this is done, the second part we have is actually
> replying to these frames. This is obviously complicated by the fact that
>
> a) we need to look at frames that are re-transmitted within the AP
> context (e.g.
> broadcast from a STA to the DS, and potentially STA-to-STA frames)
> b) we need to drop ARP requests (all in your patch, but maybe only
> handled
> ones?) - this also applies to the ones in (a)
>
> Actually (a) might also affect the first part where we build the database?
> The whole "use ARP frames to build database" part isn't very clear to me.
>
> Since we're moving to have more things like this, and more high-level
> protocol integration, I think we should instead extend ip/eb/nftables.
> It seems that much of this can be implemented in userspace already,
> with the exception of the two complications above (at least (b) - (a)
> might not be an issue depending on what we need.)
This is exactly the dilemma that I ran into while trying to do this
elsewhere. If we are talking about every IP entity that passes
through (in/out of) this AP's bridge and maintaining a database for all
of them, it could be implemented more easily (though, not sure what the
performance would look like). However, the whole "AP context"
complicates things quite a bit. As you can probably see from the code,
currently, the frame handling is done through the Tx path of the AP,
meaning the bridge has already done its job; either a STA-to-STA
traffic coming back to the same "port" or a STA's broadcast frame going
back out to the AP "port" as well as to all of the interfaces that are
attached to the bridge. This allows the implemented code to be isolated
from the rest of the ecosystem. For a simplicity sake, this is good. As
for the elegance of the design, perhaps not.
Honestly, I have no problem doing this elsewhere as long as:
1) We can add an AP context to ip/eb/nftables in a clean way. Maybe get
the hostapd involved somehow? Get it to inform the MAC addresses of
the STAs to ip/eb/nftables? A specific detail would be helpful for
me in doing the implementation. Greatly appreciated in advance.
2) The performance hit from such method would be comparable to that
of the proposed method. I feel that the current implementation's
performance hit should be minimal to the wireless stack.
Thanks!
Kyeyoon
��.n��������+%������w��{.n�����{���zW����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
