On Sat, Jun 10, 2023 at 10:18:34PM +0000, David Laight wrote: > From: Lorenzo Stoakes > > Sent: 10 June 2023 22:07 > ... > > > > OK, as per the pedantic test bot, you'll need to change this to:- > > > > > > > > num = min_t(size_t, remains, PAGE_SIZE); > > > > > > > Ordinarily I wouldn't respond to this (I go into why I feel this is not > > useful commentary below) but I am concerned Lu will take you seriously. > > > > > There has to be a valid reason why min/max have strong type checks. > > > > I really don't know what you mean by this? Yes there is a reason, I imagine > > it's to avoid unfortunate and invalid type comparisons. > > Indeed, the 'unfortunate conversion' is a negative value > being converted to a large positive one. > That doesn't require anything like the type checking that min/max do. Yes this is precisely what I thought it ought to be protecting again > > > This is not applicable here (explained below...) > > > > > Using min_t() all the time is just subverting them and means that > > > bugs are more likely than if the extra tests in min() were absent. > > > > 'All the time' - are you just having a general whine + moan about perceived > > kernel practices? Can you please keep it focused on the actual issues at > > hand? I am not Linus and therefore not responsible for the entirety of the > > kernel. > > I see a general problem (that Linus ought to worried about) > is that whenever min() reports a type error the answer is > do immediately drop in a min_t() instead of looking at the > type of the values and fixing them to that min() doesn't complain. > (Or fixing min() so it doesn't object to a much larger class > of comparisons.0 Sure, but it's not really relevant here for the reasons I went into. Probably as Andrew says elsewhere in the thread, PAGE_SIZE not being size_t is pretty annoying. > > ... > > > A 'safe' change is min(remains + 0ULL, PAGE_SIZE). > > > > So now we're promoting an unsigned int (and sometimes unsigned long of > > course) to an unsigned long long (for reasons unknown) and comparing it > > with an unsigned long? Wouldn't this trigger the sensitive type check > > anyway? > > PAGE size is defined to be 'long long' - so min() will be happy. > The compiler will just DTRT, even if 'remains' is 32bit it will > optimise away all the implied 64-bit extension. It's unsigned long in every arch I've seen right? And in a 32-bit arch long long will be 64-bit, so I think you'd still have the error here. > > Almost all the min_t() are min_t((some unsigned type),a,b). > If the values are known to be positive then: > #define min_unsigned(a, b) min((a) + 0u + 0ul + 0ull, (b) + 0u + 0ul + 0ull)) > will zero extend whatever type is supplied before the comparison. > The compiler will just discard zero extensions. > > > To be clear, I'd nack any such ridiculous change unless a hugely compelling > > reason is given (you've not given any). That's horrific. And again, you've > > not provided one single example of an _actual_ bug or situation where the > > 'problem' you tiresomely raise would occur. > > The (size_t) cast isn't in itself a problem - provided you've > checked that it is larger than the types of both sides. > But search the kernel and you'll find places when min_t((u8),a,b) > is used. > This all follows the same pattern of min() gave a warning so > so use min_t(). Yes obviously instances of inappropriate narrowing are horrible, but that isn't happening here. In this specific instance, for any actual architecture in reality there is no issue. I do absolutely agree we should address the instances where an inappropriate type is used. > > ... > > > But, in reality, min/max should always be valid when one > > > value is a constant between 0 and MAX_INT. > > > > This is getting at a signed/unsigned comparison issue here afaict which is > > not the one we're dealing with here. > > But it is exactly what min() is checking for and almost why min() > exists. > A min_unsafe() that didn't try to do any checks would be better > than train wreck that min_t() can create. All this ultimately sounds like a broader criticism of the min/max type checks and not really relevant to this patch, that should be addressed at a more general level. > > ... > > Now since you kicked off this 'all the time' stuff I feel like I have been > > given permission to make some broad comments myself... > > > > David, I am not one to commit-shame being a minor contributor myself buuuut > > I see 7,610 messages from you on lore and 4 commits, all from 4 years ago > > (please correct me if I'm wrong). > > I don't work for google, intel, aws (etc). > Getting patches accepted is surprisingly hard. > Yes, sorry, I was probably a little too mean there (long hot day and up too early) I didn't mean to be personal, what I was saying in blunt fashion is the commentary needs to be proportionate to the problem and placed at the right position, I don't feel this is. By the way I can relate on this fully as I'm a 100% hobbyist who does this part time (and writing a book on mm, yes I should get out more), and I know how difficult it can be to get patches in! > I've been writing device driver and comms protocol stack code > for best part of 40 years. Yes again apologies, I really didn't mean anything personal, I just found the review a little frustrating. You are certainly more experienced than I am! :) Cheers, Lorenzo > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
