On Mon, Feb 22, 2021 at 08:26:10AM -0800, Randy Dunlap wrote:
> On 2/22/21 7:58 AM, Jason Gunthorpe wrote:
> > On Mon, Feb 22, 2021 at 03:00:03PM +0200, Leon Romanovsky wrote:
> >> On Mon, Feb 22, 2021 at 10:39:20AM +0800, Zhu Yanjun wrote:
> >>> On Sun, Feb 21, 2021 at 2:49 PM Leon Romanovsky <[email protected]> wrote:
> >>>>
> >>>> On Fri, Feb 19, 2021 at 06:32:26PM -0500, Julian Braha wrote:
> >>>>> commit 6e61907779ba99af785f5b2397a84077c289888a
> >>>>> Author: Julian Braha <[email protected]>
> >>>>> Date: Fri Feb 19 18:20:57 2021 -0500
> >>>>>
> >>>>> drivers: infiniband: sw: rxe: fix kconfig dependency on CRYPTO
> >>>>>
> >>>>> When RDMA_RXE is enabled and CRYPTO is disabled,
> >>>>> Kbuild gives the following warning:
> >>>>>
> >>>>> WARNING: unmet direct dependencies detected for CRYPTO_CRC32
> >>>>> Depends on [n]: CRYPTO [=n]
> >>>>> Selected by [y]:
> >>>>> - RDMA_RXE [=y] && (INFINIBAND_USER_ACCESS [=y] || !INFINIBAND_USER_ACCESS [=y]) && INET [=y] && PCI [=y] && INFINIBAND [=y] && INFINIBAND_VIRT_DMA [=y]
> >>>>>
> >>>>> This is because RDMA_RXE selects CRYPTO_CRC32,
> >>>>> without depending on or selecting CRYPTO, despite that config option
> >>>>> being subordinate to CRYPTO.
> >>>>>
> >>>>> Signed-off-by: Julian Braha <[email protected]>
> >>>>
> >>>> Please use git sent-email to send patches and please fix crypto Kconfig
> >>>> to enable CRYPTO if CRYPTO_* selected.
> >>>>
> >>>> It is a little bit awkward to request all users of CRYPTO_* to request
> >>>> select CRYPTO too.
> >>>
> >>> The same issue and similar patch is in this link:
> >>>
> >>> https://patchwork.kernel.org/project/linux-rdma/patch/[email protected]/#23615747
> >>
> >> So what prevents us from fixing CRYPTO Kconfig?
> >
> > Yes, I would like to see someone deal with this properly, either every
> > place doing select CRYPTO_XX needs fixing or something needs to be
> > done in the crypto layer.
> >
> > I have no idea about kconfig to give advice, I've added Arnd since he
> > always seems to know :)
>
> I will Ack the original patch in this thread.
The one from Julian?
> How many Mellanox drivers are you concerned about?
?? This is about rxe
> You don't have to fix any other drivers that have a similar issue.
Why shouldn't they be fixed too?
There is nearly 1000 places that use a 'select CRYPTO_*' in the
kernel.
I see only 60 'select CRYPTO' statements.
Jason
On Mon, Feb 22, 2021 at 12:46:45PM -0400, Jason Gunthorpe wrote:
> On Mon, Feb 22, 2021 at 08:26:10AM -0800, Randy Dunlap wrote:
> > On 2/22/21 7:58 AM, Jason Gunthorpe wrote:
> > > On Mon, Feb 22, 2021 at 03:00:03PM +0200, Leon Romanovsky wrote:
> > >> On Mon, Feb 22, 2021 at 10:39:20AM +0800, Zhu Yanjun wrote:
> > >>> On Sun, Feb 21, 2021 at 2:49 PM Leon Romanovsky <[email protected]> wrote:
> > >>>>
> > >>>> On Fri, Feb 19, 2021 at 06:32:26PM -0500, Julian Braha wrote:
> > >>>>> commit 6e61907779ba99af785f5b2397a84077c289888a
> > >>>>> Author: Julian Braha <[email protected]>
> > >>>>> Date: Fri Feb 19 18:20:57 2021 -0500
> > >>>>>
> > >>>>> drivers: infiniband: sw: rxe: fix kconfig dependency on CRYPTO
> > >>>>>
> > >>>>> When RDMA_RXE is enabled and CRYPTO is disabled,
> > >>>>> Kbuild gives the following warning:
> > >>>>>
> > >>>>> WARNING: unmet direct dependencies detected for CRYPTO_CRC32
> > >>>>> Depends on [n]: CRYPTO [=n]
> > >>>>> Selected by [y]:
> > >>>>> - RDMA_RXE [=y] && (INFINIBAND_USER_ACCESS [=y] || !INFINIBAND_USER_ACCESS [=y]) && INET [=y] && PCI [=y] && INFINIBAND [=y] && INFINIBAND_VIRT_DMA [=y]
> > >>>>>
> > >>>>> This is because RDMA_RXE selects CRYPTO_CRC32,
> > >>>>> without depending on or selecting CRYPTO, despite that config option
> > >>>>> being subordinate to CRYPTO.
> > >>>>>
> > >>>>> Signed-off-by: Julian Braha <[email protected]>
> > >>>>
> > >>>> Please use git sent-email to send patches and please fix crypto Kconfig
> > >>>> to enable CRYPTO if CRYPTO_* selected.
> > >>>>
> > >>>> It is a little bit awkward to request all users of CRYPTO_* to request
> > >>>> select CRYPTO too.
> > >>>
> > >>> The same issue and similar patch is in this link:
> > >>>
> > >>> https://patchwork.kernel.org/project/linux-rdma/patch/[email protected]/#23615747
> > >>
> > >> So what prevents us from fixing CRYPTO Kconfig?
> > >
> > > Yes, I would like to see someone deal with this properly, either every
> > > place doing select CRYPTO_XX needs fixing or something needs to be
> > > done in the crypto layer.
> > >
> > > I have no idea about kconfig to give advice, I've added Arnd since he
> > > always seems to know :)
> >
> > I will Ack the original patch in this thread.
>
> The one from Julian?
>
> > How many Mellanox drivers are you concerned about?
>
> ?? This is about rxe
>
> > You don't have to fix any other drivers that have a similar issue.
>
> Why shouldn't they be fixed too?
>
> There is nearly 1000 places that use a 'select CRYPTO_*' in the
> kernel.
>
> I see only 60 'select CRYPTO' statements.
I don't like the suggestion to ack and not fix either.
All CRYPTO_CRC32C users need CRYPTO and it means that CRYPTO Kconfig
should be fixed.
➜ kernel git:(queue-next) git grep -B 1 "select CRYPTO_CRC32"
drivers/infiniband/sw/rxe/Kconfig- select NET_UDP_TUNNEL
drivers/infiniband/sw/rxe/Kconfig: select CRYPTO_CRC32
--
drivers/nvme/host/Kconfig- select CRYPTO
drivers/nvme/host/Kconfig: select CRYPTO_CRC32C
--
drivers/scsi/Kconfig- select CRYPTO_MD5
drivers/scsi/Kconfig: select CRYPTO_CRC32C
--
drivers/target/iscsi/Kconfig- select CRYPTO
drivers/target/iscsi/Kconfig: select CRYPTO_CRC32C
drivers/target/iscsi/Kconfig: select CRYPTO_CRC32C_INTEL if X86
--
fs/btrfs/Kconfig- select CRYPTO
fs/btrfs/Kconfig: select CRYPTO_CRC32C
--
fs/ext4/Kconfig- select CRYPTO
fs/ext4/Kconfig: select CRYPTO_CRC32C
--
fs/f2fs/Kconfig- select CRYPTO
fs/f2fs/Kconfig: select CRYPTO_CRC32
--
fs/jbd2/Kconfig- select CRYPTO
fs/jbd2/Kconfig: select CRYPTO_CRC32C
--
lib/Kconfig- select CRYPTO
lib/Kconfig: select CRYPTO_CRC32C
>
> Jason
On 2/22/21 8:46 AM, Jason Gunthorpe wrote:
> On Mon, Feb 22, 2021 at 08:26:10AM -0800, Randy Dunlap wrote:
>> On 2/22/21 7:58 AM, Jason Gunthorpe wrote:
>>> On Mon, Feb 22, 2021 at 03:00:03PM +0200, Leon Romanovsky wrote:
>>>> On Mon, Feb 22, 2021 at 10:39:20AM +0800, Zhu Yanjun wrote:
>>>>> On Sun, Feb 21, 2021 at 2:49 PM Leon Romanovsky <[email protected]> wrote:
>>>>>>
>>>>>> On Fri, Feb 19, 2021 at 06:32:26PM -0500, Julian Braha wrote:
>>>>>>> commit 6e61907779ba99af785f5b2397a84077c289888a
>>>>>>> Author: Julian Braha <[email protected]>
>>>>>>> Date: Fri Feb 19 18:20:57 2021 -0500
>>>>>>>
>>>>>>> drivers: infiniband: sw: rxe: fix kconfig dependency on CRYPTO
>>>>>>>
>>>>>>> When RDMA_RXE is enabled and CRYPTO is disabled,
>>>>>>> Kbuild gives the following warning:
>>>>>>>
>>>>>>> WARNING: unmet direct dependencies detected for CRYPTO_CRC32
>>>>>>> Depends on [n]: CRYPTO [=n]
>>>>>>> Selected by [y]:
>>>>>>> - RDMA_RXE [=y] && (INFINIBAND_USER_ACCESS [=y] || !INFINIBAND_USER_ACCESS [=y]) && INET [=y] && PCI [=y] && INFINIBAND [=y] && INFINIBAND_VIRT_DMA [=y]
>>>>>>>
>>>>>>> This is because RDMA_RXE selects CRYPTO_CRC32,
>>>>>>> without depending on or selecting CRYPTO, despite that config option
>>>>>>> being subordinate to CRYPTO.
>>>>>>>
>>>>>>> Signed-off-by: Julian Braha <[email protected]>
>>>>>>
>>>>>> Please use git sent-email to send patches and please fix crypto Kconfig
>>>>>> to enable CRYPTO if CRYPTO_* selected.
>>>>>>
>>>>>> It is a little bit awkward to request all users of CRYPTO_* to request
>>>>>> select CRYPTO too.
>>>>>
>>>>> The same issue and similar patch is in this link:
>>>>>
>>>>> https://patchwork.kernel.org/project/linux-rdma/patch/[email protected]/#23615747
>>>>
>>>> So what prevents us from fixing CRYPTO Kconfig?
>>>
>>> Yes, I would like to see someone deal with this properly, either every
>>> place doing select CRYPTO_XX needs fixing or something needs to be
>>> done in the crypto layer.
>>>
>>> I have no idea about kconfig to give advice, I've added Arnd since he
>>> always seems to know :)
>>
>> I will Ack the original patch in this thread.
>
> The one from Julian?
Yes.
>
>> How many Mellanox drivers are you concerned about?
>
> ?? This is about rxe
OK.
>> You don't have to fix any other drivers that have a similar issue.
>
> Why shouldn't they be fixed too?
Of course, but it's not just on you and/or Leon to fix them.
> There is nearly 1000 places that use a 'select CRYPTO_*' in the
> kernel.
>
> I see only 60 'select CRYPTO' statements.
Correct. :(
We (community) tend to fix bug reports, not do global audits
to see what needs to be fixed (with a few exceptions).
I'll gladly wait to see what Arnd says.
--
~Randy
On Mon, Feb 22, 2021 at 5:53 PM Randy Dunlap <[email protected]> wrote:
> On 2/22/21 8:46 AM, Jason Gunthorpe wrote:
>
> > There is nearly 1000 places that use a 'select CRYPTO_*' in the
> > kernel.
> >
> > I see only 60 'select CRYPTO' statements.
I think most of these are correct, as you typically have a single
'select CRYPTO'
in combination with a couple of 'select CRYPTO_*' ones for the specific
algorithms. I just added a lot of 'select CRC32' statements to deal with
all the build failures in drivers that require this but fail to have an extra
select statement. The way I got the list was to start with 'make allmodconfig'
and then recursively disable everything that had an explicit select, until
I was left with all the modules that need it without selecting it.
The same method could be used for CONFIG_CRYPTO, but it's a few
hours of work.
> Correct. :(
> We (community) tend to fix bug reports, not do global audits
> to see what needs to be fixed (with a few exceptions).
>
>
> I'll gladly wait to see what Arnd says.
For the specific case of CRC32, it might actually a good idea to change
the code to call into the CRC32 code directly instead of the CRYPTO_CRC32
abstraction. Would that work for RDMA_RXE?
Arnd
On Tue, Feb 23, 2021 at 9:36 PM Arnd Bergmann <[email protected]> wrote:
>
> For the specific case of CRC32, it might actually a good idea to change
> the code to call into the CRC32 code directly instead of the CRYPTO_CRC32
> abstraction. Would that work for RDMA_RXE?
On the more general question of whether a driver should 'select CRYPTO',
this is how it's currently done for the other users, but I don't
actually like this,
and in general recommend against force-enabling another subsystem when
a particular driver is enabled.
My preference would be to change all drivers that require crypto services
of some kind to use 'depends on CRYPTO' in combination with 'select CRYPTO_*',
as this is what we do for other cross-subsystem dependencies. However,
it seems unlikely that we can change it anytime soon, as the current method
is widespread and changing the dependencies would break users that do
'make oldconfig' on an old configuration.
Arnd