Hi Vladimir, sorry for the delay, getting answers to all you questions seems to be challenging for me. Unfortunately it's about 1 year ago when I was originally working on this particular problem and obviously I didn't understand the full problem...
On Wednesday, 14 October 2020, 19:31:03 CEST, Vladimir Oltean wrote: > On Wed, Oct 14, 2020 at 07:02:13PM +0200, Christian Eggers wrote: > > > Otherwise said, the frame must be padded to > > > max(skb->len, ETH_ZLEN) + tail tag length. > > > > At first I thought the same when working on this. But IMHO the padding > > must > > only ensure the minimum required size, there is no need to pad to the > > "real" size of the skb. The check for the tailroom above ensures that > > enough memory for the "real" size is available. > > Yes, that's right, that's the current logic, but what's the point of > your patch, then, if the call to __skb_put_padto is only supposed to > ensure ETH_ZLEN length? After checking __skb_put_padto, I realized that I didn't knew what it actually does. The problem in my case is, that the condition skb_tailroom(skb) >= padlen + len may not be met anymore after __skb_put_padto(..., skb->len + padlen, ...) returns. If skb has previously been cloned, __skb_put_padto() will allocate a copy which may have a size of equal / only slightly more than skb->len + padlen, which misses the full space for the tail tag. Further calls to skb_put() may not find enough tailroom for placing the tail tag. > In fact, __skb_put_padto fundamentally does: > - an extension of skb->len to the requested argument, via __skb_put > - a zero-filling of the extra area > So if you include the length of the tag in the call to __skb_put_padto, > then what's the other skb_put() from ksz8795_xmit, ksz9477_xmit, > ksz9893_xmit going to do? Aren't you increasing the frame length twice > by the length of one tag when you are doing this? I initially thought that __skb_put_padto() would perform some sort of allocation which can later be used by skb_put(). You are right that my change would increase the frame twice. The only reason why this worked for me was because the newly allocated skb had enough tailroom due to alignment. > What problem are you actually trying to solve? After (hopefully) understanding the important bits, I would like to solve the problem that after calling __skb_put_padto() there may be no tailroom for the tail tag. The conditions where this can happen are quite special. You need a skb->len < ETH_ZLEN and the skb must be marked as cloned. One condition where this happens in practice is when the skb has been selected for TX time stamping in dsa_skb_tx_timestamp() [cloned] and L2 is used as transport for PTP [size < ETH_ZLEN]. But maybe cloned sk_buffs can also happen for other reasons. I now suggest the following: - if (skb_tailroom(skb) >= padlen + len) { + if (skb_tailroom(skb) >= padlen + len && !skb_cloned(skb)) { This would avoid allocation of a new skb in __skb_put_padto() which may be finally too small. > Can you show a skb_dump(KERN_ERR, skb, true) before and after your change? Best regards Christian