Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9da7f41e-6d4a-452a-8042-0b09cad71bb8@quicinc.com>
Date:   Wed, 30 Aug 2023 13:22:37 -0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/2] mac80211: Use flexible array in struct
 ieee80211_tim_ie
Content-Language: en-US
To:     Christian Lamparter <chunkeey@gmail.com>, <kernel@quicinc.com>,
        Kalle Valo <kvalo@kernel.org>,
        =?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@toke.dk>,
        Christian Lamparter <chunkeey@googlemail.com>,
        "Stanislaw Gruszka" <stf_xl@wp.pl>,
        Helmut Schaa <helmut.schaa@googlemail.com>,
        "Ping-Ke Shih" <pkshih@realtek.com>,
        Johannes Berg <johannes@sipsolutions.net>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        "Jakub Kicinski" <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Kees Cook <keescook@chromium.org>
CC:     <linux-wireless@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <netdev@vger.kernel.org>
References: <20230829-ieee80211_tim_ie-v2-0-fdaf19fb1c0e@quicinc.com>
 <20230829-ieee80211_tim_ie-v2-2-fdaf19fb1c0e@quicinc.com>
 <1774098a-5062-4f12-a760-f16036d095e3@gmail.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <1774098a-5062-4f12-a760-f16036d095e3@gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 8/30/2023 12:51 PM, Christian Lamparter wrote:
> Hi,
> 
> On 8/29/23 15:29, Jeff Johnson wrote:
>> Currently struct ieee80211_tim_ie defines:
>>     u8 virtual_map[1];
>>
>> Per the guidance in [1] change this to be a flexible array.
>>
>> As a result of this change, adjust all related struct size tests to
>> account for the fact that the sizeof(struct ieee80211_tim_ie) now
>> accounts for the minimum size of the virtual_map.
>>
>> [1] 
>> https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
>>
>> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
>> ---
>> diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
>> index bd2f6e19c357..4cdc2eb98f16 100644
>> --- a/include/linux/ieee80211.h
>> +++ b/include/linux/ieee80211.h
>> @@ -961,7 +961,7 @@ struct ieee80211_tim_ie {
>>       u8 dtim_period;
>>       u8 bitmap_ctrl;
>>       /* variable size: 1 - 251 bytes */
>> -    u8 virtual_map[1];
>> +    u8 virtual_map[];
>>   } __packed;
> 
> 
> Uhh, the 802.11 (my 2012 Version has this in) spec in
> 8.4.2.7 TIM Element demands this to be 1 - 251 bytes.
> And this is why there's a comment above... With your
> change this could be confusing. Would it be possible
> to fix that somehow? Like in a anonymous union/group
> with a flexible array and a u8?

Adding Kees to the discussion for any advice. Yes, the virtual_map must 
contain at least one octet but may contain more than one. And to 
complicate matters, the information that tells us how many octets are 
actually present is found outside the struct; the TLV header that 
precedes the struct will contain the length of the struct, and hence the 
length of the bitmap is that size - 2 (the size of the dtim_period and 
bitmap_ctrl fields).

I don't think it is unique to this struct that an 802.11 element will 
have optional octets for which one or more octets must always be 
present. For that reason I've been updating ieee80211.h kdoc to point to 
the latest specification since that ultimately provides the necessary 
guidance.

/jeff