[libcamera-devel,v2,1/2] libcamera: ipa_manager: Only parse IPA modules
diff mbox series

Message ID 20230504144801.2590668-2-kieran.bingham@ideasonboard.com
State Rejected
Headers show
Series
  • V4L2 Compatibility installation directory
Related show

Commit Message

Kieran Bingham May 4, 2023, 2:48 p.m. UTC 
Ensure that when we iterate the libcamera libdir we only select shared
objects which are expected to be IPA modules, with an ipa_ prefix.

Any shared object not matching this filter is ignored and not processed
by the IPA Manager.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/ipa_manager.cpp | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Laurent Pinchart May 4, 2023, 3:10 p.m. UTC | #1 
Hi Kieran,

Thank you for the patch.

On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote:
> Ensure that when we iterate the libcamera libdir we only select shared
> objects which are expected to be IPA modules, with an ipa_ prefix.
> 
> Any shared object not matching this filter is ignored and not processed
> by the IPA Manager.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

I wonder what this protects against, as there shouldn't be other files
in this directory, especially if we move IPA modules to an ipa/
subdirectory. I don't mind much, as this isn't a hot path, so I have no
objection if you want to merge this, but is it useful ?

> ---
>  src/libcamera/ipa_manager.cpp | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 030ef43fb994..c0c2f027e902 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -206,6 +206,10 @@ void IPAManager::parseDir(const char *libDir, unsigned int maxDepth,
>  		if (strcmp(&ent->d_name[offset], ".so"))
>  			continue;
>  
> +		/* Ignore any modules which are not IPAs. */
> +		if (strncmp(ent->d_name, "ipa_", 4) != 0)
> +			continue;
> +
>  		files.push_back(std::string(libDir) + "/" + ent->d_name);
>  	}
>
Kieran Bingham May 4, 2023, 7:03 p.m. UTC | #2 
On 04/05/2023 16:10, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote:
>> Ensure that when we iterate the libcamera libdir we only select shared
>> objects which are expected to be IPA modules, with an ipa_ prefix.
>>
>> Any shared object not matching this filter is ignored and not processed
>> by the IPA Manager.
>>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> I wonder what this protects against, as there shouldn't be other files
> in this directory, especially if we move IPA modules to an ipa/
> subdirectory. I don't mind much, as this isn't a hot path, so I have no
> objection if you want to merge this, but is it useful ?

Only that it prevents us trying to load any files that might be there 
otherwise.



[76:32:31.897778214] [2776180] ERROR IPAModule ipa_module.cpp:286 
fake.so: IPA module is not an ELF file
[76:32:31.899112311] [2776180] ERROR IPAModule ipa_module.cpp:172 Symbol 
ipaModuleInfo not found
[76:32:31.899119975] [2776180] ERROR IPAModule ipa_module.cpp:292 
v4l2-compat.so: IPA module has no valid info


We already filter on .so objects, and we do other checks so it's not 
crucial, and it's not going to prevent any attempt to get 
'non-libcamera' code parsed as anyone dropping a file here could equally 
call it 'ipa_fake.so' ... so no not really.

I'll drop this and collect 2/2

--
Kieran



> 
>> ---
>>   src/libcamera/ipa_manager.cpp | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>> index 030ef43fb994..c0c2f027e902 100644
>> --- a/src/libcamera/ipa_manager.cpp
>> +++ b/src/libcamera/ipa_manager.cpp
>> @@ -206,6 +206,10 @@ void IPAManager::parseDir(const char *libDir, unsigned int maxDepth,
>>   		if (strcmp(&ent->d_name[offset], ".so"))
>>   			continue;
>>   
>> +		/* Ignore any modules which are not IPAs. */
>> +		if (strncmp(ent->d_name, "ipa_", 4) != 0)
>> +			continue;
>> +
>>   		files.push_back(std::string(libDir) + "/" + ent->d_name);
>>   	}
>>   
>
Kieran Bingham Aug. 28, 2023, 12:28 p.m. UTC | #3 
Quoting Kieran Bingham (2023-05-04 20:03:56)
> On 04/05/2023 16:10, Laurent Pinchart wrote:
> > Hi Kieran,
> > 
> > Thank you for the patch.
> > 
> > On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote:
> >> Ensure that when we iterate the libcamera libdir we only select shared
> >> objects which are expected to be IPA modules, with an ipa_ prefix.
> >>
> >> Any shared object not matching this filter is ignored and not processed
> >> by the IPA Manager.
> >>
> >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > I wonder what this protects against, as there shouldn't be other files
> > in this directory, especially if we move IPA modules to an ipa/
> > subdirectory. I don't mind much, as this isn't a hot path, so I have no
> > objection if you want to merge this, but is it useful ?
> 
> Only that it prevents us trying to load any files that might be there 
> otherwise.
> 
> 
> 
> [76:32:31.897778214] [2776180] ERROR IPAModule ipa_module.cpp:286 
> fake.so: IPA module is not an ELF file
> [76:32:31.899112311] [2776180] ERROR IPAModule ipa_module.cpp:172 Symbol 
> ipaModuleInfo not found
> [76:32:31.899119975] [2776180] ERROR IPAModule ipa_module.cpp:292 
> v4l2-compat.so: IPA module has no valid info
> 
> 
> We already filter on .so objects, and we do other checks so it's not 
> crucial, and it's not going to prevent any attempt to get 
> 'non-libcamera' code parsed as anyone dropping a file here could equally 
> call it 'ipa_fake.so' ... so no not really.
> 
> I'll drop this and collect 2/2

I've started seeing various logs that report similar to:
```
$ wireplumber 
[12:34:30.015061894] [206341] ERROR IPAModule ipa_module.cpp:172 Symbol ipaModuleInfo not found
[12:34:30.015085339] [206341] ERROR IPAModule ipa_module.cpp:292 v4l2-compat.so: IPA module has no valid info
[12:34:30.015125806] [206341]  INFO Camera camera_manager.cpp:284 libcamera v0.1.0
```

I believe this is because some distributions are probably overriding the
installation path of the v4l2-compat.so.

Anyway, that makes me think that we /should/ ignore files that are not
prefixed by ipa_ ...


--
Kieran


> 
> --
> Kieran
> 
> 
> 
> > 
> >> ---
> >>   src/libcamera/ipa_manager.cpp | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> >> index 030ef43fb994..c0c2f027e902 100644
> >> --- a/src/libcamera/ipa_manager.cpp
> >> +++ b/src/libcamera/ipa_manager.cpp
> >> @@ -206,6 +206,10 @@ void IPAManager::parseDir(const char *libDir, unsigned int maxDepth,
> >>              if (strcmp(&ent->d_name[offset], ".so"))
> >>                      continue;
> >>   
> >> +            /* Ignore any modules which are not IPAs. */
> >> +            if (strncmp(ent->d_name, "ipa_", 4) != 0)
> >> +                    continue;
> >> +
> >>              files.push_back(std::string(libDir) + "/" + ent->d_name);
> >>      }
> >>   
> >
Laurent Pinchart Aug. 28, 2023, 7:13 p.m. UTC | #4 
On Mon, Aug 28, 2023 at 01:28:37PM +0100, Kieran Bingham wrote:
> Quoting Kieran Bingham (2023-05-04 20:03:56)
> > On 04/05/2023 16:10, Laurent Pinchart wrote:
> > > On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote:
> > >> Ensure that when we iterate the libcamera libdir we only select shared
> > >> objects which are expected to be IPA modules, with an ipa_ prefix.
> > >>
> > >> Any shared object not matching this filter is ignored and not processed
> > >> by the IPA Manager.
> > >>
> > >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > 
> > > I wonder what this protects against, as there shouldn't be other files
> > > in this directory, especially if we move IPA modules to an ipa/
> > > subdirectory. I don't mind much, as this isn't a hot path, so I have no
> > > objection if you want to merge this, but is it useful ?
> > 
> > Only that it prevents us trying to load any files that might be there 
> > otherwise.
> > 
> > 
> > [76:32:31.897778214] [2776180] ERROR IPAModule ipa_module.cpp:286 
> > fake.so: IPA module is not an ELF file
> > [76:32:31.899112311] [2776180] ERROR IPAModule ipa_module.cpp:172 Symbol 
> > ipaModuleInfo not found
> > [76:32:31.899119975] [2776180] ERROR IPAModule ipa_module.cpp:292 
> > v4l2-compat.so: IPA module has no valid info
> > 
> > 
> > We already filter on .so objects, and we do other checks so it's not 
> > crucial, and it's not going to prevent any attempt to get 
> > 'non-libcamera' code parsed as anyone dropping a file here could equally 
> > call it 'ipa_fake.so' ... so no not really.
> > 
> > I'll drop this and collect 2/2
> 
> I've started seeing various logs that report similar to:
> ```
> $ wireplumber 
> [12:34:30.015061894] [206341] ERROR IPAModule ipa_module.cpp:172 Symbol ipaModuleInfo not found
> [12:34:30.015085339] [206341] ERROR IPAModule ipa_module.cpp:292 v4l2-compat.so: IPA module has no valid info
> [12:34:30.015125806] [206341]  INFO Camera camera_manager.cpp:284 libcamera v0.1.0
> ```
> 
> I believe this is because some distributions are probably overriding the
> installation path of the v4l2-compat.so.
> 
> Anyway, that makes me think that we /should/ ignore files that are not
> prefixed by ipa_ ...

Or move IPA modules to an ipa/ subdirectory ?

> > >> ---
> > >>   src/libcamera/ipa_manager.cpp | 4 ++++
> > >>   1 file changed, 4 insertions(+)
> > >>
> > >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> > >> index 030ef43fb994..c0c2f027e902 100644
> > >> --- a/src/libcamera/ipa_manager.cpp
> > >> +++ b/src/libcamera/ipa_manager.cpp
> > >> @@ -206,6 +206,10 @@ void IPAManager::parseDir(const char *libDir, unsigned int maxDepth,
> > >>              if (strcmp(&ent->d_name[offset], ".so"))
> > >>                      continue;
> > >>   
> > >> +            /* Ignore any modules which are not IPAs. */
> > >> +            if (strncmp(ent->d_name, "ipa_", 4) != 0)
> > >> +                    continue;
> > >> +
> > >>              files.push_back(std::string(libDir) + "/" + ent->d_name);
> > >>      }
> > >>

Patch
diff mbox series 

diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
index 030ef43fb994..c0c2f027e902 100644
--- a/src/libcamera/ipa_manager.cpp
+++ b/src/libcamera/ipa_manager.cpp
@@ -206,6 +206,10 @@  void IPAManager::parseDir(const char *libDir, unsigned int maxDepth,
 		if (strcmp(&ent->d_name[offset], ".so"))
 			continue;
 
+		/* Ignore any modules which are not IPAs. */
+		if (strncmp(ent->d_name, "ipa_", 4) != 0)
+			continue;
+
 		files.push_back(std::string(libDir) + "/" + ent->d_name);
 	}