Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 29-9-2016 23:57, Rafał Miłecki wrote: > On 27 September 2016 at 11:24, Arend Van Spriel >wrote: >> On 26-9-2016 14:38, Rafał Miłecki wrote: >>> On 26 September 2016 at 14:13, Rafał Miłecki wrote: On 26 September 2016 at 13:46, Arend Van Spriel wrote: > On 26-9-2016 12:23, Rafał Miłecki wrote: >> From: Rafał Miłecki >> >> We need to track 802.1x packets to know if there are any pending ones >> for transmission. This is required for performing key update in the >> firmware. > > The problem we are trying to solve is a pretty old one. The problem is > that wpa_supplicant uses two separate code paths: EAPOL messaging > through data path and key configuration though nl80211. Can I find it described/reported somewhere? >> Unfortunately our old tracking code wasn't very accurate. It was >> treating skb as pending as soon as it was passed by the netif. Actual >> handling packet to the firmware was happening later as brcmfmac >> internally queues them and uses its own worker(s). > > That does not seem right. As soon as we get a 1x packet we need to wait > with key configuration regardless whether it is still in the driver or > handed over to firmware already. OK, thanks. >>> >>> Actually, it's not OK. I was trying to report/describe/discuss this >>> problem for over a week. I couldn't get much of answer from you. >>> >>> I had to come with a patch I worked on for quite some time. Only then >>> you decided to react and reply with a reason for a nack. I see this >>> patch may be wrong (but it's still hard to know what's going wrong >>> without a proper hostapd bug report). I'd expect you to somehow work & >>> communicate with open source community. >> >> We do or at least make an honest attempt, but there is more on our plate >> so responses may be delayed. It also does not help when you get anal and >> preachy when we do respond. Also not OK. In this case the delay is >> caused because I had to pick up the thread(s) as Hante is on vacation >> (he needed a break :-p ). However, you started sending patches so I >> decided to look at and respond to those. Sorry if you felt like we left >> you hanging to dry. > > I believe I get easily irritated due to my communication experience I > got so far :( > > > Over a year ago I reported brcmfmac can't recover from failed > register_netdev(ice). This bug remains unfixed. > > In 2014 I reported problem with 80 MHz support. I didn't have hardware > to fix & test it on my own (you weren't able/allowed to send me one of > your PCIe cards). In remained broken until I fixed it year later. > > You missed my crash bug report about caused by missing eth_type_trans > and came with patch on your own a month later. > > Earlier this year I reported you problem with BCM4366 and multiple > interfaces. I didn't get much help. 3 months later I came with patch > to workaround the problem but you said there's a better way to do > this. It took me 2 weeks to figure out a new wlioctl API for that > while all I needed was a simple hint on "interface_remove". > > Right now I'm waiting to get any answer from you about 4366c0 > firmware. It's still less than 2 weeks since I asked for it, but a > simple ETA would be nice. I'm actually not sure if I should report > more problems to you to don't distract you from pending things. This is a difficult question. All upstream firmware releases for router chips are put on hold until further notice. Some decisions have been made, but I have not seen a detailed plan to give an ETA. > Problems with brcmf_netdev_wait_pend8021x were reported multiples > times for last few months. When I finally got time for that it took me > a week to debug them. For the pend8021x you were sending a number of messages showing debug progress so not sure whether you wanted our feedback on that. If so a ping might have done it. > As you can see, it takes me months to get help on some things. And in > few cases I never got much help at all. Yes, I was hoping to have you > more involved into brcmfmac development and problems solving. I guess > things didn't meet my expectations and I got grumpy & preachy. Thanks for listing all our failures. Somehow we are very good at getting each other grumpy. When we provide a patch and you break it up and submit that to Kalle, we get grumpy and it all piles up to the point where we have this kind of conversation. As long as it helps to get things of our chest I can live with that. Hope you can too. We strive to give support to the community, but the priority is low as it is not full-time activity. Regards, Arend
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 29-9-2016 23:57, Rafał Miłecki wrote: > On 27 September 2016 at 11:24, Arend Van Spriel > wrote: >> On 26-9-2016 14:38, Rafał Miłecki wrote: >>> On 26 September 2016 at 14:13, Rafał Miłecki wrote: On 26 September 2016 at 13:46, Arend Van Spriel wrote: > On 26-9-2016 12:23, Rafał Miłecki wrote: >> From: Rafał Miłecki >> >> We need to track 802.1x packets to know if there are any pending ones >> for transmission. This is required for performing key update in the >> firmware. > > The problem we are trying to solve is a pretty old one. The problem is > that wpa_supplicant uses two separate code paths: EAPOL messaging > through data path and key configuration though nl80211. Can I find it described/reported somewhere? >> Unfortunately our old tracking code wasn't very accurate. It was >> treating skb as pending as soon as it was passed by the netif. Actual >> handling packet to the firmware was happening later as brcmfmac >> internally queues them and uses its own worker(s). > > That does not seem right. As soon as we get a 1x packet we need to wait > with key configuration regardless whether it is still in the driver or > handed over to firmware already. OK, thanks. >>> >>> Actually, it's not OK. I was trying to report/describe/discuss this >>> problem for over a week. I couldn't get much of answer from you. >>> >>> I had to come with a patch I worked on for quite some time. Only then >>> you decided to react and reply with a reason for a nack. I see this >>> patch may be wrong (but it's still hard to know what's going wrong >>> without a proper hostapd bug report). I'd expect you to somehow work & >>> communicate with open source community. >> >> We do or at least make an honest attempt, but there is more on our plate >> so responses may be delayed. It also does not help when you get anal and >> preachy when we do respond. Also not OK. In this case the delay is >> caused because I had to pick up the thread(s) as Hante is on vacation >> (he needed a break :-p ). However, you started sending patches so I >> decided to look at and respond to those. Sorry if you felt like we left >> you hanging to dry. > > I believe I get easily irritated due to my communication experience I > got so far :( > > > Over a year ago I reported brcmfmac can't recover from failed > register_netdev(ice). This bug remains unfixed. > > In 2014 I reported problem with 80 MHz support. I didn't have hardware > to fix & test it on my own (you weren't able/allowed to send me one of > your PCIe cards). In remained broken until I fixed it year later. > > You missed my crash bug report about caused by missing eth_type_trans > and came with patch on your own a month later. > > Earlier this year I reported you problem with BCM4366 and multiple > interfaces. I didn't get much help. 3 months later I came with patch > to workaround the problem but you said there's a better way to do > this. It took me 2 weeks to figure out a new wlioctl API for that > while all I needed was a simple hint on "interface_remove". > > Right now I'm waiting to get any answer from you about 4366c0 > firmware. It's still less than 2 weeks since I asked for it, but a > simple ETA would be nice. I'm actually not sure if I should report > more problems to you to don't distract you from pending things. This is a difficult question. All upstream firmware releases for router chips are put on hold until further notice. Some decisions have been made, but I have not seen a detailed plan to give an ETA. > Problems with brcmf_netdev_wait_pend8021x were reported multiples > times for last few months. When I finally got time for that it took me > a week to debug them. For the pend8021x you were sending a number of messages showing debug progress so not sure whether you wanted our feedback on that. If so a ping might have done it. > As you can see, it takes me months to get help on some things. And in > few cases I never got much help at all. Yes, I was hoping to have you > more involved into brcmfmac development and problems solving. I guess > things didn't meet my expectations and I got grumpy & preachy. Thanks for listing all our failures. Somehow we are very good at getting each other grumpy. When we provide a patch and you break it up and submit that to Kalle, we get grumpy and it all piles up to the point where we have this kind of conversation. As long as it helps to get things of our chest I can live with that. Hope you can too. We strive to give support to the community, but the priority is low as it is not full-time activity. Regards, Arend
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 27 September 2016 at 11:24, Arend Van Sprielwrote: > On 26-9-2016 14:38, Rafał Miłecki wrote: >> On 26 September 2016 at 14:13, Rafał Miłecki wrote: >>> On 26 September 2016 at 13:46, Arend Van Spriel >>> wrote: On 26-9-2016 12:23, Rafał Miłecki wrote: > From: Rafał Miłecki > > We need to track 802.1x packets to know if there are any pending ones > for transmission. This is required for performing key update in the > firmware. The problem we are trying to solve is a pretty old one. The problem is that wpa_supplicant uses two separate code paths: EAPOL messaging through data path and key configuration though nl80211. >>> >>> Can I find it described/reported somewhere? >>> >>> > Unfortunately our old tracking code wasn't very accurate. It was > treating skb as pending as soon as it was passed by the netif. Actual > handling packet to the firmware was happening later as brcmfmac > internally queues them and uses its own worker(s). That does not seem right. As soon as we get a 1x packet we need to wait with key configuration regardless whether it is still in the driver or handed over to firmware already. >>> >>> OK, thanks. >> >> Actually, it's not OK. I was trying to report/describe/discuss this >> problem for over a week. I couldn't get much of answer from you. >> >> I had to come with a patch I worked on for quite some time. Only then >> you decided to react and reply with a reason for a nack. I see this >> patch may be wrong (but it's still hard to know what's going wrong >> without a proper hostapd bug report). I'd expect you to somehow work & >> communicate with open source community. > > We do or at least make an honest attempt, but there is more on our plate > so responses may be delayed. It also does not help when you get anal and > preachy when we do respond. Also not OK. In this case the delay is > caused because I had to pick up the thread(s) as Hante is on vacation > (he needed a break :-p ). However, you started sending patches so I > decided to look at and respond to those. Sorry if you felt like we left > you hanging to dry. I believe I get easily irritated due to my communication experience I got so far :( Over a year ago I reported brcmfmac can't recover from failed register_netdev(ice). This bug remains unfixed. In 2014 I reported problem with 80 MHz support. I didn't have hardware to fix & test it on my own (you weren't able/allowed to send me one of your PCIe cards). In remained broken until I fixed it year later. You missed my crash bug report about caused by missing eth_type_trans and came with patch on your own a month later. Earlier this year I reported you problem with BCM4366 and multiple interfaces. I didn't get much help. 3 months later I came with patch to workaround the problem but you said there's a better way to do this. It took me 2 weeks to figure out a new wlioctl API for that while all I needed was a simple hint on "interface_remove". Right now I'm waiting to get any answer from you about 4366c0 firmware. It's still less than 2 weeks since I asked for it, but a simple ETA would be nice. I'm actually not sure if I should report more problems to you to don't distract you from pending things. Problems with brcmf_netdev_wait_pend8021x were reported multiples times for last few months. When I finally got time for that it took me a week to debug them. As you can see, it takes me months to get help on some things. And in few cases I never got much help at all. Yes, I was hoping to have you more involved into brcmfmac development and problems solving. I guess things didn't meet my expectations and I got grumpy & preachy. -- Rafał
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 27 September 2016 at 11:24, Arend Van Spriel wrote: > On 26-9-2016 14:38, Rafał Miłecki wrote: >> On 26 September 2016 at 14:13, Rafał Miłecki wrote: >>> On 26 September 2016 at 13:46, Arend Van Spriel >>> wrote: On 26-9-2016 12:23, Rafał Miłecki wrote: > From: Rafał Miłecki > > We need to track 802.1x packets to know if there are any pending ones > for transmission. This is required for performing key update in the > firmware. The problem we are trying to solve is a pretty old one. The problem is that wpa_supplicant uses two separate code paths: EAPOL messaging through data path and key configuration though nl80211. >>> >>> Can I find it described/reported somewhere? >>> >>> > Unfortunately our old tracking code wasn't very accurate. It was > treating skb as pending as soon as it was passed by the netif. Actual > handling packet to the firmware was happening later as brcmfmac > internally queues them and uses its own worker(s). That does not seem right. As soon as we get a 1x packet we need to wait with key configuration regardless whether it is still in the driver or handed over to firmware already. >>> >>> OK, thanks. >> >> Actually, it's not OK. I was trying to report/describe/discuss this >> problem for over a week. I couldn't get much of answer from you. >> >> I had to come with a patch I worked on for quite some time. Only then >> you decided to react and reply with a reason for a nack. I see this >> patch may be wrong (but it's still hard to know what's going wrong >> without a proper hostapd bug report). I'd expect you to somehow work & >> communicate with open source community. > > We do or at least make an honest attempt, but there is more on our plate > so responses may be delayed. It also does not help when you get anal and > preachy when we do respond. Also not OK. In this case the delay is > caused because I had to pick up the thread(s) as Hante is on vacation > (he needed a break :-p ). However, you started sending patches so I > decided to look at and respond to those. Sorry if you felt like we left > you hanging to dry. I believe I get easily irritated due to my communication experience I got so far :( Over a year ago I reported brcmfmac can't recover from failed register_netdev(ice). This bug remains unfixed. In 2014 I reported problem with 80 MHz support. I didn't have hardware to fix & test it on my own (you weren't able/allowed to send me one of your PCIe cards). In remained broken until I fixed it year later. You missed my crash bug report about caused by missing eth_type_trans and came with patch on your own a month later. Earlier this year I reported you problem with BCM4366 and multiple interfaces. I didn't get much help. 3 months later I came with patch to workaround the problem but you said there's a better way to do this. It took me 2 weeks to figure out a new wlioctl API for that while all I needed was a simple hint on "interface_remove". Right now I'm waiting to get any answer from you about 4366c0 firmware. It's still less than 2 weeks since I asked for it, but a simple ETA would be nice. I'm actually not sure if I should report more problems to you to don't distract you from pending things. Problems with brcmf_netdev_wait_pend8021x were reported multiples times for last few months. When I finally got time for that it took me a week to debug them. As you can see, it takes me months to get help on some things. And in few cases I never got much help at all. Yes, I was hoping to have you more involved into brcmfmac development and problems solving. I guess things didn't meet my expectations and I got grumpy & preachy. -- Rafał
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 26-9-2016 14:38, Rafał Miłecki wrote: > On 26 September 2016 at 14:13, Rafał Miłeckiwrote: >> On 26 September 2016 at 13:46, Arend Van Spriel >> wrote: >>> On 26-9-2016 12:23, Rafał Miłecki wrote: From: Rafał Miłecki We need to track 802.1x packets to know if there are any pending ones for transmission. This is required for performing key update in the firmware. >>> >>> The problem we are trying to solve is a pretty old one. The problem is >>> that wpa_supplicant uses two separate code paths: EAPOL messaging >>> through data path and key configuration though nl80211. >> >> Can I find it described/reported somewhere? >> >> Unfortunately our old tracking code wasn't very accurate. It was treating skb as pending as soon as it was passed by the netif. Actual handling packet to the firmware was happening later as brcmfmac internally queues them and uses its own worker(s). >>> >>> That does not seem right. As soon as we get a 1x packet we need to wait >>> with key configuration regardless whether it is still in the driver or >>> handed over to firmware already. >> >> OK, thanks. > > Actually, it's not OK. I was trying to report/describe/discuss this > problem for over a week. I couldn't get much of answer from you. > > I had to come with a patch I worked on for quite some time. Only then > you decided to react and reply with a reason for a nack. I see this > patch may be wrong (but it's still hard to know what's going wrong > without a proper hostapd bug report). I'd expect you to somehow work & > communicate with open source community. We do or at least make an honest attempt, but there is more on our plate so responses may be delayed. It also does not help when you get anal and preachy when we do respond. Also not OK. In this case the delay is caused because I had to pick up the thread(s) as Hante is on vacation (he needed a break :-p ). However, you started sending patches so I decided to look at and respond to those. Sorry if you felt like we left you hanging to dry. Regards, Arend
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 26-9-2016 14:38, Rafał Miłecki wrote: > On 26 September 2016 at 14:13, Rafał Miłecki wrote: >> On 26 September 2016 at 13:46, Arend Van Spriel >> wrote: >>> On 26-9-2016 12:23, Rafał Miłecki wrote: From: Rafał Miłecki We need to track 802.1x packets to know if there are any pending ones for transmission. This is required for performing key update in the firmware. >>> >>> The problem we are trying to solve is a pretty old one. The problem is >>> that wpa_supplicant uses two separate code paths: EAPOL messaging >>> through data path and key configuration though nl80211. >> >> Can I find it described/reported somewhere? >> >> Unfortunately our old tracking code wasn't very accurate. It was treating skb as pending as soon as it was passed by the netif. Actual handling packet to the firmware was happening later as brcmfmac internally queues them and uses its own worker(s). >>> >>> That does not seem right. As soon as we get a 1x packet we need to wait >>> with key configuration regardless whether it is still in the driver or >>> handed over to firmware already. >> >> OK, thanks. > > Actually, it's not OK. I was trying to report/describe/discuss this > problem for over a week. I couldn't get much of answer from you. > > I had to come with a patch I worked on for quite some time. Only then > you decided to react and reply with a reason for a nack. I see this > patch may be wrong (but it's still hard to know what's going wrong > without a proper hostapd bug report). I'd expect you to somehow work & > communicate with open source community. We do or at least make an honest attempt, but there is more on our plate so responses may be delayed. It also does not help when you get anal and preachy when we do respond. Also not OK. In this case the delay is caused because I had to pick up the thread(s) as Hante is on vacation (he needed a break :-p ). However, you started sending patches so I decided to look at and respond to those. Sorry if you felt like we left you hanging to dry. Regards, Arend
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 26-9-2016 16:59, Dan Williams wrote: > On Mon, 2016-09-26 at 14:13 +0200, Rafał Miłecki wrote: >> On 26 September 2016 at 13:46, Arend Van Spriel >>wrote: >>> >>> On 26-9-2016 12:23, Rafał Miłecki wrote: From: Rafał Miłecki We need to track 802.1x packets to know if there are any pending ones for transmission. This is required for performing key update in the firmware. >>> >>> The problem we are trying to solve is a pretty old one. The problem >>> is >>> that wpa_supplicant uses two separate code paths: EAPOL messaging >>> through data path and key configuration though nl80211. >> >> Can I find it described/reported somewhere? > > If I understand the issue correctly, you can find all this in the > supplicant code. Once the supplicant has done whatever it wants to do > with the data frames that just happen to be EAPOL it then sends the > keys down to the driver with nl80211. Indeed. EAPOL packets are simply data packets as far as the 802.11 stack is concerned. The arrival of those in the driver is not predictable hence we hold off the key configuration until those have been passed over to firmware. > But it sounds like, instead of sniffing EAPOL frames in the driver skb > tracking and sniffing ETH_P_PAE, you should probably implement support > for NL80211_CMD_CRIT_PROTOCOL_START/NL80211_CMD_CRIT_PROTOCOL_STOP and > key off the passed-in NL80211_CRIT_PROTO_EAPOL. At least at the > beginning of connection setup only EAPOL packets will be allowed > anyway. > > It doesn't seem like the supplicant uses NL80211_CRIT_PROTO_EAPOL yet, > but that should also be fixed in the supplicant itself. You should > probably get some comments from Jouni on how he'd like to see all this > work. But generally the less specific sniffing of frames in drivers, > likely the better. Indeed. That was the main motivation to introduce the CRIT_PROTO api. If I recall correctly it was considered the task of the network manager to issue the START/STOP. Recently noticed the use of CRIT_PROTO_DHCP on some target system, which we already support in brcmfmac. From your response I guess you consider CRIT_PROTO_EAPOL to be issued by the supplicant. Regards, Arend > Dan > >> >>> Unfortunately our old tracking code wasn't very accurate. It was treating skb as pending as soon as it was passed by the netif. Actual handling packet to the firmware was happening later as brcmfmac internally queues them and uses its own worker(s). >>> >>> That does not seem right. As soon as we get a 1x packet we need to >>> wait >>> with key configuration regardless whether it is still in the driver >>> or >>> handed over to firmware already. >> >> OK, thanks.
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 26-9-2016 16:59, Dan Williams wrote: > On Mon, 2016-09-26 at 14:13 +0200, Rafał Miłecki wrote: >> On 26 September 2016 at 13:46, Arend Van Spriel >> wrote: >>> >>> On 26-9-2016 12:23, Rafał Miłecki wrote: From: Rafał Miłecki We need to track 802.1x packets to know if there are any pending ones for transmission. This is required for performing key update in the firmware. >>> >>> The problem we are trying to solve is a pretty old one. The problem >>> is >>> that wpa_supplicant uses two separate code paths: EAPOL messaging >>> through data path and key configuration though nl80211. >> >> Can I find it described/reported somewhere? > > If I understand the issue correctly, you can find all this in the > supplicant code. Once the supplicant has done whatever it wants to do > with the data frames that just happen to be EAPOL it then sends the > keys down to the driver with nl80211. Indeed. EAPOL packets are simply data packets as far as the 802.11 stack is concerned. The arrival of those in the driver is not predictable hence we hold off the key configuration until those have been passed over to firmware. > But it sounds like, instead of sniffing EAPOL frames in the driver skb > tracking and sniffing ETH_P_PAE, you should probably implement support > for NL80211_CMD_CRIT_PROTOCOL_START/NL80211_CMD_CRIT_PROTOCOL_STOP and > key off the passed-in NL80211_CRIT_PROTO_EAPOL. At least at the > beginning of connection setup only EAPOL packets will be allowed > anyway. > > It doesn't seem like the supplicant uses NL80211_CRIT_PROTO_EAPOL yet, > but that should also be fixed in the supplicant itself. You should > probably get some comments from Jouni on how he'd like to see all this > work. But generally the less specific sniffing of frames in drivers, > likely the better. Indeed. That was the main motivation to introduce the CRIT_PROTO api. If I recall correctly it was considered the task of the network manager to issue the START/STOP. Recently noticed the use of CRIT_PROTO_DHCP on some target system, which we already support in brcmfmac. From your response I guess you consider CRIT_PROTO_EAPOL to be issued by the supplicant. Regards, Arend > Dan > >> >>> Unfortunately our old tracking code wasn't very accurate. It was treating skb as pending as soon as it was passed by the netif. Actual handling packet to the firmware was happening later as brcmfmac internally queues them and uses its own worker(s). >>> >>> That does not seem right. As soon as we get a 1x packet we need to >>> wait >>> with key configuration regardless whether it is still in the driver >>> or >>> handed over to firmware already. >> >> OK, thanks.
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On Mon, 2016-09-26 at 14:13 +0200, Rafał Miłecki wrote: > On 26 September 2016 at 13:46, Arend Van Spriel >wrote: > > > > On 26-9-2016 12:23, Rafał Miłecki wrote: > > > > > > From: Rafał Miłecki > > > > > > We need to track 802.1x packets to know if there are any pending > > > ones > > > for transmission. This is required for performing key update in > > > the > > > firmware. > > > > The problem we are trying to solve is a pretty old one. The problem > > is > > that wpa_supplicant uses two separate code paths: EAPOL messaging > > through data path and key configuration though nl80211. > > Can I find it described/reported somewhere? If I understand the issue correctly, you can find all this in the supplicant code. Once the supplicant has done whatever it wants to do with the data frames that just happen to be EAPOL it then sends the keys down to the driver with nl80211. But it sounds like, instead of sniffing EAPOL frames in the driver skb tracking and sniffing ETH_P_PAE, you should probably implement support for NL80211_CMD_CRIT_PROTOCOL_START/NL80211_CMD_CRIT_PROTOCOL_STOP and key off the passed-in NL80211_CRIT_PROTO_EAPOL. At least at the beginning of connection setup only EAPOL packets will be allowed anyway. It doesn't seem like the supplicant uses NL80211_CRIT_PROTO_EAPOL yet, but that should also be fixed in the supplicant itself. You should probably get some comments from Jouni on how he'd like to see all this work. But generally the less specific sniffing of frames in drivers, likely the better. Dan > > > > > > > > > Unfortunately our old tracking code wasn't very accurate. It was > > > treating skb as pending as soon as it was passed by the netif. > > > Actual > > > handling packet to the firmware was happening later as brcmfmac > > > internally queues them and uses its own worker(s). > > > > That does not seem right. As soon as we get a 1x packet we need to > > wait > > with key configuration regardless whether it is still in the driver > > or > > handed over to firmware already. > > OK, thanks.
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On Mon, 2016-09-26 at 14:13 +0200, Rafał Miłecki wrote: > On 26 September 2016 at 13:46, Arend Van Spriel > wrote: > > > > On 26-9-2016 12:23, Rafał Miłecki wrote: > > > > > > From: Rafał Miłecki > > > > > > We need to track 802.1x packets to know if there are any pending > > > ones > > > for transmission. This is required for performing key update in > > > the > > > firmware. > > > > The problem we are trying to solve is a pretty old one. The problem > > is > > that wpa_supplicant uses two separate code paths: EAPOL messaging > > through data path and key configuration though nl80211. > > Can I find it described/reported somewhere? If I understand the issue correctly, you can find all this in the supplicant code. Once the supplicant has done whatever it wants to do with the data frames that just happen to be EAPOL it then sends the keys down to the driver with nl80211. But it sounds like, instead of sniffing EAPOL frames in the driver skb tracking and sniffing ETH_P_PAE, you should probably implement support for NL80211_CMD_CRIT_PROTOCOL_START/NL80211_CMD_CRIT_PROTOCOL_STOP and key off the passed-in NL80211_CRIT_PROTO_EAPOL. At least at the beginning of connection setup only EAPOL packets will be allowed anyway. It doesn't seem like the supplicant uses NL80211_CRIT_PROTO_EAPOL yet, but that should also be fixed in the supplicant itself. You should probably get some comments from Jouni on how he'd like to see all this work. But generally the less specific sniffing of frames in drivers, likely the better. Dan > > > > > > > > > Unfortunately our old tracking code wasn't very accurate. It was > > > treating skb as pending as soon as it was passed by the netif. > > > Actual > > > handling packet to the firmware was happening later as brcmfmac > > > internally queues them and uses its own worker(s). > > > > That does not seem right. As soon as we get a 1x packet we need to > > wait > > with key configuration regardless whether it is still in the driver > > or > > handed over to firmware already. > > OK, thanks.
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 26 September 2016 at 14:13, Rafał Miłeckiwrote: > On 26 September 2016 at 13:46, Arend Van Spriel > wrote: >> On 26-9-2016 12:23, Rafał Miłecki wrote: >>> From: Rafał Miłecki >>> >>> We need to track 802.1x packets to know if there are any pending ones >>> for transmission. This is required for performing key update in the >>> firmware. >> >> The problem we are trying to solve is a pretty old one. The problem is >> that wpa_supplicant uses two separate code paths: EAPOL messaging >> through data path and key configuration though nl80211. > > Can I find it described/reported somewhere? > > >>> Unfortunately our old tracking code wasn't very accurate. It was >>> treating skb as pending as soon as it was passed by the netif. Actual >>> handling packet to the firmware was happening later as brcmfmac >>> internally queues them and uses its own worker(s). >> >> That does not seem right. As soon as we get a 1x packet we need to wait >> with key configuration regardless whether it is still in the driver or >> handed over to firmware already. > > OK, thanks. Actually, it's not OK. I was trying to report/describe/discuss this problem for over a week. I couldn't get much of answer from you. I had to come with a patch I worked on for quite some time. Only then you decided to react and reply with a reason for a nack. I see this patch may be wrong (but it's still hard to know what's going wrong without a proper hostapd bug report). I'd expect you to somehow work & communicate with open source community. -- Rafał
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 26 September 2016 at 14:13, Rafał Miłecki wrote: > On 26 September 2016 at 13:46, Arend Van Spriel > wrote: >> On 26-9-2016 12:23, Rafał Miłecki wrote: >>> From: Rafał Miłecki >>> >>> We need to track 802.1x packets to know if there are any pending ones >>> for transmission. This is required for performing key update in the >>> firmware. >> >> The problem we are trying to solve is a pretty old one. The problem is >> that wpa_supplicant uses two separate code paths: EAPOL messaging >> through data path and key configuration though nl80211. > > Can I find it described/reported somewhere? > > >>> Unfortunately our old tracking code wasn't very accurate. It was >>> treating skb as pending as soon as it was passed by the netif. Actual >>> handling packet to the firmware was happening later as brcmfmac >>> internally queues them and uses its own worker(s). >> >> That does not seem right. As soon as we get a 1x packet we need to wait >> with key configuration regardless whether it is still in the driver or >> handed over to firmware already. > > OK, thanks. Actually, it's not OK. I was trying to report/describe/discuss this problem for over a week. I couldn't get much of answer from you. I had to come with a patch I worked on for quite some time. Only then you decided to react and reply with a reason for a nack. I see this patch may be wrong (but it's still hard to know what's going wrong without a proper hostapd bug report). I'd expect you to somehow work & communicate with open source community. -- Rafał
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 26-9-2016 14:13, Rafał Miłecki wrote: > On 26 September 2016 at 13:46, Arend Van Spriel >wrote: >> On 26-9-2016 12:23, Rafał Miłecki wrote: >>> From: Rafał Miłecki >>> >>> We need to track 802.1x packets to know if there are any pending ones >>> for transmission. This is required for performing key update in the >>> firmware. >> >> The problem we are trying to solve is a pretty old one. The problem is >> that wpa_supplicant uses two separate code paths: EAPOL messaging >> through data path and key configuration though nl80211. > > Can I find it described/reported somewhere? Not sure. It is something that I recall from working at Intersil so back in the prism days. Regards, Arend >>> Unfortunately our old tracking code wasn't very accurate. It was >>> treating skb as pending as soon as it was passed by the netif. Actual >>> handling packet to the firmware was happening later as brcmfmac >>> internally queues them and uses its own worker(s). >> >> That does not seem right. As soon as we get a 1x packet we need to wait >> with key configuration regardless whether it is still in the driver or >> handed over to firmware already. > > OK, thanks. >
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 26-9-2016 14:13, Rafał Miłecki wrote: > On 26 September 2016 at 13:46, Arend Van Spriel > wrote: >> On 26-9-2016 12:23, Rafał Miłecki wrote: >>> From: Rafał Miłecki >>> >>> We need to track 802.1x packets to know if there are any pending ones >>> for transmission. This is required for performing key update in the >>> firmware. >> >> The problem we are trying to solve is a pretty old one. The problem is >> that wpa_supplicant uses two separate code paths: EAPOL messaging >> through data path and key configuration though nl80211. > > Can I find it described/reported somewhere? Not sure. It is something that I recall from working at Intersil so back in the prism days. Regards, Arend >>> Unfortunately our old tracking code wasn't very accurate. It was >>> treating skb as pending as soon as it was passed by the netif. Actual >>> handling packet to the firmware was happening later as brcmfmac >>> internally queues them and uses its own worker(s). >> >> That does not seem right. As soon as we get a 1x packet we need to wait >> with key configuration regardless whether it is still in the driver or >> handed over to firmware already. > > OK, thanks. >
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 26 September 2016 at 13:46, Arend Van Sprielwrote: > On 26-9-2016 12:23, Rafał Miłecki wrote: >> From: Rafał Miłecki >> >> We need to track 802.1x packets to know if there are any pending ones >> for transmission. This is required for performing key update in the >> firmware. > > The problem we are trying to solve is a pretty old one. The problem is > that wpa_supplicant uses two separate code paths: EAPOL messaging > through data path and key configuration though nl80211. Can I find it described/reported somewhere? >> Unfortunately our old tracking code wasn't very accurate. It was >> treating skb as pending as soon as it was passed by the netif. Actual >> handling packet to the firmware was happening later as brcmfmac >> internally queues them and uses its own worker(s). > > That does not seem right. As soon as we get a 1x packet we need to wait > with key configuration regardless whether it is still in the driver or > handed over to firmware already. OK, thanks.
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 26 September 2016 at 13:46, Arend Van Spriel wrote: > On 26-9-2016 12:23, Rafał Miłecki wrote: >> From: Rafał Miłecki >> >> We need to track 802.1x packets to know if there are any pending ones >> for transmission. This is required for performing key update in the >> firmware. > > The problem we are trying to solve is a pretty old one. The problem is > that wpa_supplicant uses two separate code paths: EAPOL messaging > through data path and key configuration though nl80211. Can I find it described/reported somewhere? >> Unfortunately our old tracking code wasn't very accurate. It was >> treating skb as pending as soon as it was passed by the netif. Actual >> handling packet to the firmware was happening later as brcmfmac >> internally queues them and uses its own worker(s). > > That does not seem right. As soon as we get a 1x packet we need to wait > with key configuration regardless whether it is still in the driver or > handed over to firmware already. OK, thanks.
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 26-9-2016 12:23, Rafał Miłecki wrote: > From: Rafał Miłecki> > We need to track 802.1x packets to know if there are any pending ones > for transmission. This is required for performing key update in the > firmware. The problem we are trying to solve is a pretty old one. The problem is that wpa_supplicant uses two separate code paths: EAPOL messaging through data path and key configuration though nl80211. > Unfortunately our old tracking code wasn't very accurate. It was > treating skb as pending as soon as it was passed by the netif. Actual > handling packet to the firmware was happening later as brcmfmac > internally queues them and uses its own worker(s). That does not seem right. As soon as we get a 1x packet we need to wait with key configuration regardless whether it is still in the driver or handed over to firmware already. Regards, Arend > Other than that it was hard to handle freeing packets. Everytime we had > to determine (in more generic funcions) if packet was counted as pending > 802.1x one or not. It was causing some problems, e.g. it wasn't clear if > brcmf_flowring_delete should free skb directly or not. > > This patch introduces 2 separated functions for tracking skbs. This > simplifies logic, fixes brcmf_flowring_delete (maybe other hidden bugs > as well) and allows further simplifications. Thanks to better accuracy > is also increases time window for key update (and lowers timeout risk). > > Signed-off-by: Rafał Miłecki > --- > This was successfully tested with 4366b1. Can someone give it a try with > some USB/SDIO device, please? > --- > .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c| 11 +++ > .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 12 +++- > .../wireless/broadcom/brcm80211/brcmfmac/core.c| 36 > -- > .../wireless/broadcom/brcm80211/brcmfmac/core.h| 2 ++ > .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c | 14 +++-- > .../wireless/broadcom/brcm80211/brcmfmac/proto.h | 11 +++ > .../wireless/broadcom/brcm80211/brcmfmac/sdio.c| 8 + > .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 10 ++ > 8 files changed, 91 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c > index d1bc51f..3e40244 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c > @@ -326,6 +326,16 @@ brcmf_proto_bcdc_hdrpull(struct brcmf_pub *drvr, bool > do_fws, > return 0; > } > > +static int brcmf_proto_bcdc_hdr_get_ifidx(struct brcmf_pub *drvr, > + struct sk_buff *skb) > +{ > + struct brcmf_proto_bcdc_header *h; > + > + h = (struct brcmf_proto_bcdc_header *)(skb->data); > + > + return BCDC_GET_IF_IDX(h); > +} > + > static int > brcmf_proto_bcdc_txdata(struct brcmf_pub *drvr, int ifidx, u8 offset, > struct sk_buff *pktbuf) > @@ -373,6 +383,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr) > } > > drvr->proto->hdrpull = brcmf_proto_bcdc_hdrpull; > + drvr->proto->hdr_get_ifidx = brcmf_proto_bcdc_hdr_get_ifidx; > drvr->proto->query_dcmd = brcmf_proto_bcdc_query_dcmd; > drvr->proto->set_dcmd = brcmf_proto_bcdc_set_dcmd; > drvr->proto->txdata = brcmf_proto_bcdc_txdata; > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > index 03404cb..fef9d02 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > @@ -43,6 +43,7 @@ > #include "chip.h" > #include "bus.h" > #include "debug.h" > +#include "proto.h" > #include "sdio.h" > #include "core.h" > #include "common.h" > @@ -772,6 +773,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, > u8 *buf, uint nbytes) > int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev, >struct sk_buff_head *pktq) > { > + struct brcmf_pub *pub = sdiodev->bus_if->drvr; > struct sk_buff *skb; > u32 addr = sdiodev->sbwad; > int err; > @@ -784,10 +786,18 @@ int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev, > > if (pktq->qlen == 1 || !sdiodev->sg_support) > skb_queue_walk(pktq, skb) { > + struct brcmf_if *ifp; > + int ifidx; > + > + ifidx = brcmf_proto_hdr_get_ifidx(pub, skb); > + ifp = brcmf_get_ifp(pub, ifidx); > + brcmf_tx_passing_skb(ifp, skb); > err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, true, >addr, skb); > - if (err) > + if (err) { > +
Re: [PATCH] brcmfmac: implement more accurate skb tracking
On 26-9-2016 12:23, Rafał Miłecki wrote: > From: Rafał Miłecki > > We need to track 802.1x packets to know if there are any pending ones > for transmission. This is required for performing key update in the > firmware. The problem we are trying to solve is a pretty old one. The problem is that wpa_supplicant uses two separate code paths: EAPOL messaging through data path and key configuration though nl80211. > Unfortunately our old tracking code wasn't very accurate. It was > treating skb as pending as soon as it was passed by the netif. Actual > handling packet to the firmware was happening later as brcmfmac > internally queues them and uses its own worker(s). That does not seem right. As soon as we get a 1x packet we need to wait with key configuration regardless whether it is still in the driver or handed over to firmware already. Regards, Arend > Other than that it was hard to handle freeing packets. Everytime we had > to determine (in more generic funcions) if packet was counted as pending > 802.1x one or not. It was causing some problems, e.g. it wasn't clear if > brcmf_flowring_delete should free skb directly or not. > > This patch introduces 2 separated functions for tracking skbs. This > simplifies logic, fixes brcmf_flowring_delete (maybe other hidden bugs > as well) and allows further simplifications. Thanks to better accuracy > is also increases time window for key update (and lowers timeout risk). > > Signed-off-by: Rafał Miłecki > --- > This was successfully tested with 4366b1. Can someone give it a try with > some USB/SDIO device, please? > --- > .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c| 11 +++ > .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 12 +++- > .../wireless/broadcom/brcm80211/brcmfmac/core.c| 36 > -- > .../wireless/broadcom/brcm80211/brcmfmac/core.h| 2 ++ > .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c | 14 +++-- > .../wireless/broadcom/brcm80211/brcmfmac/proto.h | 11 +++ > .../wireless/broadcom/brcm80211/brcmfmac/sdio.c| 8 + > .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 10 ++ > 8 files changed, 91 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c > index d1bc51f..3e40244 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c > @@ -326,6 +326,16 @@ brcmf_proto_bcdc_hdrpull(struct brcmf_pub *drvr, bool > do_fws, > return 0; > } > > +static int brcmf_proto_bcdc_hdr_get_ifidx(struct brcmf_pub *drvr, > + struct sk_buff *skb) > +{ > + struct brcmf_proto_bcdc_header *h; > + > + h = (struct brcmf_proto_bcdc_header *)(skb->data); > + > + return BCDC_GET_IF_IDX(h); > +} > + > static int > brcmf_proto_bcdc_txdata(struct brcmf_pub *drvr, int ifidx, u8 offset, > struct sk_buff *pktbuf) > @@ -373,6 +383,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr) > } > > drvr->proto->hdrpull = brcmf_proto_bcdc_hdrpull; > + drvr->proto->hdr_get_ifidx = brcmf_proto_bcdc_hdr_get_ifidx; > drvr->proto->query_dcmd = brcmf_proto_bcdc_query_dcmd; > drvr->proto->set_dcmd = brcmf_proto_bcdc_set_dcmd; > drvr->proto->txdata = brcmf_proto_bcdc_txdata; > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > index 03404cb..fef9d02 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > @@ -43,6 +43,7 @@ > #include "chip.h" > #include "bus.h" > #include "debug.h" > +#include "proto.h" > #include "sdio.h" > #include "core.h" > #include "common.h" > @@ -772,6 +773,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, > u8 *buf, uint nbytes) > int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev, >struct sk_buff_head *pktq) > { > + struct brcmf_pub *pub = sdiodev->bus_if->drvr; > struct sk_buff *skb; > u32 addr = sdiodev->sbwad; > int err; > @@ -784,10 +786,18 @@ int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev, > > if (pktq->qlen == 1 || !sdiodev->sg_support) > skb_queue_walk(pktq, skb) { > + struct brcmf_if *ifp; > + int ifidx; > + > + ifidx = brcmf_proto_hdr_get_ifidx(pub, skb); > + ifp = brcmf_get_ifp(pub, ifidx); > + brcmf_tx_passing_skb(ifp, skb); > err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, true, >addr, skb); > - if (err) > + if (err) { > + brcmf_tx_regained_skb(ifp, skb); >